Update parsetree.py removed "?" from for x in re.compile(r"(\${.+})" …

[jjgalvez]

…which was preventing a dict from being consumed as an expression in ${}

In relation to the discussion on how to pass a dict to an expression, removing the ? from the regular expression on line 325 for parsetree.py allows ${} to consume expressions with also contain "{","}" in the expression.
Please consider accepting this pull-request.

[zzzeek]

hi -

it is essential that tests be added , in in this case in test_lexer.py . A change like this is very high risk. See #387, #383 for examples of PRs that can be accepted.

[jjgalvez]

hi -

it is essential that tests be added , in in this case in test_lexer.py . A change like this is very high risk. See #387, #383 for examples of PRs that can be accepted.

understood I'll review the examples and add the tests. I'll create a new pull request once I've got the tests done and they are all passing.

[jjgalvez]

Would you prefer that I add new tests or add usecases to either test_expressoin or test_tricky_expression. If a new test case I could add test_dict_expression

[zzzeek]

TBH I am skeptical this patch can work since you will now fail to parse lines like ${x} ${y} properly, but yes I would say add some new tests for this

[jjgalvez]

I added tests to test_lexer, when I run the test locally I am passing all tests. including the ${x} ${y} test

[zzzeek]

OK, maybe it can work for this limited situation then...not sure about nested dictionaries? it depends on the scope of how that bracket is parsed. sort of surprised it works though, I would need to analyze very closely

[sqla-tester]

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

[sqla-tester]

New Gerrit review created for change 33a85f2: https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/5296

[jjgalvez]

OK, maybe it can work for this limited situation then...not sure about nested dictionaries? it depends on the scope of how that bracket is parsed. sort of surprised it works though, I would need to analyze very closely

I hadn't though of nested dict I will give that a try and see if it works, and update the tests as appropriate

[jjgalvez]

I updated the test to include a

OK, maybe it can work for this limited situation then...not sure about nested dictionaries? it depends on the scope of how that bracket is parsed. sort of surprised it works though, I would need to analyze very closely

I updated the test to include a dict within a dict. I have all tests passing

[jjgalvez]

OK, maybe it can work for this limited situation then...not sure about nested dictionaries? it depends on the scope of how that bracket is parsed. sort of surprised it works though, I would need to analyze very closely

Also thank you for considering the pull request and for taking such a close look at it, Given it's the lexer I know how careful you need to be.

[sqla-tester]

Michael Bayer (zzzeek) wrote:

oh this is in parsetree and not lexer. Ah OK, that makes a little more sense why it doesnt go out of control.

still surprising the lexer gets the correct tokens in the first place. i will have to find some more time to review this, i have a lot going on next week

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/5296

[sqla-tester]

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/5296 has been merged. Congratulations! :)

[zzzeek]

OK this did cause a regression, so I have to revert this.

[zzzeek]

1.3.4 is yanked. I will add a commit that adds the passing test for #401. if you want to try again, you'll have to start wiht a new PR and make sure #401's case works and we will also have to come up with a lot more.

[jjgalvez]

of course I'll keep looking at if and I can figure out a better solution I will post a new PR