Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support passing custom filters with the same name as built-in flags #413

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cocolato
Copy link
Contributor

Fix: #140

During the lexical analysis phase, add an additional prefix for undeclared identifiers that have the same name as built-in flags, and determine the final filter to be used during the code generation phase based on the context provided by the user.

from mako.template import Template

x = Template("""
X:
    ${"asdf" | h.foo}
""")

class h(object):
    foo = str

try:
    print x.render(h=h)
except NameError, e:
    print e

This code can now be rendered correctly.

Copy link
Member

@zzzeek zzzeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey there!

lots of comments. mostly looking to clean up variable names and integrate better in the filter logic, can you review a refactoring I'm proposing? I did it step by step but I'm not 100% sure i didnt miss something, tests pass for each refactor step.

mako/codegen.py Outdated Show resolved Hide resolved
mako/codegen.py Outdated Show resolved Hide resolved
mako/pyparser.py Outdated Show resolved Hide resolved
test/test_ast.py Outdated Show resolved Hide resolved
mako/filters.py Outdated Show resolved Hide resolved
mako/codegen.py Outdated Show resolved Hide resolved
mako/codegen.py Outdated Show resolved Hide resolved
@cocolato
Copy link
Contributor Author

hey there!

lots of comments. mostly looking to clean up variable names and integrate better in the filter logic, can you review a refactoring I'm proposing? I did it step by step but I'm not 100% sure i didnt miss something, tests pass for each refactor step.

Thank you for taking the time and giving me so many suggestions and comments! I'm on a short trip and don't have a computer with me at the moment. I should be back home in two days to revise the current submission again🙏🙏

@cocolato
Copy link
Contributor Author

The latest code has been updated. Previously, it might have been because I was concerned that making too many alterations to the code would impact other functions. Thus, I attempted to refrain from modifying the original logic and merely added some patch codes to it.

@cocolato
Copy link
Contributor Author

cocolato commented Oct 28, 2024

I continued to test the efficiency issue regarding text replacement. The added remove_prefix method in Python 3.9 might be a better choice, but Mako currently only minimally supports Python 3.8.

import itertools
import re

from faker import Faker 

PREFIX = "__SOME_PREFIX_"
PREFIX_REG = re.compile(r"^__SOME_PREFIX")

class MyFaker(Faker):
    def prefix_name(self):
        return f"{PREFIX}{self.name()}"

fake = MyFaker()
data_num = 10000
escape_percent = 0.5
escape_num = int(data_num * escape_percent)

prefix_texts = [fake.prefix_name() for _ in range(escape_num)]
normal_texts = [fake.name() for _ in range(data_num - escape_num)]

def test_and_replace():
    for text in itertools.chain(prefix_texts, normal_texts):
        if text.startswith(PREFIX):
            res = text.replace(PREFIX, "")
    return res

def just_replace():
    for text in itertools.chain(prefix_texts, normal_texts):
        res = text.replace(PREFIX, "")
    return res

def remove_prefix():
    for text in itertools.chain(prefix_texts, normal_texts):
        res = text.removeprefix(PREFIX)
    return res

def re_replace():
    for text in itertools.chain(prefix_texts, normal_texts):
        res = PREFIX_REG.sub(text, "")
    return res

__benchmarks__ = [
    (test_and_replace, just_replace, "just_replace"),
    (test_and_replace, remove_prefix, "remove_prefix"),
    (test_and_replace, re_replace, "re_replace")
]

benchmark res:

                                   Benchmarks, repeat=5, number=20                                   
┏━━━━━━━━━━━━━━━┳━━━━━━━━━┳━━━━━━━━━┳━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┓
┃     Benchmark ┃ Min     ┃ Max     ┃ Mean    ┃ Min (+)         ┃ Max (+)         ┃ Mean (+)        ┃
┡━━━━━━━━━━━━━━━╇━━━━━━━━━╇━━━━━━━━━╇━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━┩
│  just_replace │ 0.038   │ 0.043   │ 0.040   │ 0.018 (2.2x)    │ 0.018 (2.4x)    │ 0.018 (2.2x)    │
│ remove_prefix │ 0.038   │ 0.039   │ 0.039   │ 0.011 (3.5x)    │ 0.012 (3.3x)    │ 0.011 (3.5x)    │
│    re_replace │ 0.041   │ 0.048   │ 0.045   │ 0.034 (1.2x)    │ 0.039 (1.2x)    │ 0.037 (1.2x)    │
└───────────────┴─────────┴─────────┴─────────┴─────────────────┴─────────────────┴─────────────────┘

@zzzeek
Copy link
Member

zzzeek commented Oct 31, 2024

mainly I wanted to show that we can just have a single line with replace() and not do the extra check. the goal is mostly less code

Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 5e64e05 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

New Gerrit review created for change 5e64e05: https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/5557

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

inconsistent behavior of filter pulling from context based on name
3 participants