search

sqlalchemy / mako

Mako Templates for Python
https://www.makotemplates.org
MIT License
353 stars 60 forks source link

allow attributes to contain dictionaries #402

Closed jjgalvez closed 3 months ago

jjgalvez commented 3 months ago

I reworked the initial regular expression in the parsetree.Tag._parseattributes function to allow attributes to contain dictionaries. The difficult part was really to allow for the capture of multiple "}" while allowing for trailing white space at the end. An artifact of my regular expression is that the resulting created list elements ended up with captured white space that needed to be stripped. I added the additional use case with multiple expressions next to each other without any space between them.

jjgalvez commented 3 months ago

currently my PR is passing all the test cases including test case 400. If you have additional use cases that I should check for please let me know

zzzeek commented 3 months ago

that's my main concern is thinking of them. i dont deal with mako very much these days so I would need to consider cases. the regex is more complex now and I'm concerned it's hardcoded to specific combinations. I also dont understand the use of [{]+ instead of just {+

jjgalvez commented 3 months ago

that's my main concern is thinking of them. i dont deal with mako very much these days so I would need to consider cases. the regex is more complex now and I'm concerned it's hardcoded to specific combinations. I also dont understand the use of [{]+ instead of just {+

I completely get what you are saying. Honestly the [{]+ is likely overkill, but the closing [\s*}]* was needed to account for white space in the expression with the dictionary. I modified the code, adding an extra function looking for multiple pairs of {} which is really my use case. I then modified the _parse_attributes function to only use my complex regular expression on this limited subset and the usual one the rest of the time. I am still passing all pytest with this modified code. Hope this helps as now most if not all, except for limited use cases mako will use the original time tested regular expression.

jjgalvez commented 3 months ago

Been giving this more thought. Although I didn't "like" the solution of declaring the dict as a variable and passing that to the expression, truth is my use case is managed by Mako without my PR. Also I think I'm just adding complexity where it's unneeded. I'm going to pull this PR for now, but thanks for considering in the first place.