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 33a85f2: 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