Closed jjgalvez closed 3 months ago
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.
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.
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
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
I added tests to test_lexer, when I run the test locally I am passing all tests. including the ${x} ${y} test
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
New Gerrit review created for change 33a85f258b4904465be55eeb3e3e31a9b83648e9: https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/5296
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
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
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.
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
Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/mako/+/5296 has been merged. Congratulations! :)
OK this did cause a regression, so I have to revert this.
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.
of course I'll keep looking at if and I can figure out a better solution I will post a new PR
…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.