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

allow attributes to contain dictionaries #402

Closed
wants to merge 6 commits into from
Closed

allow attributes to contain dictionaries #402

wants to merge 6 commits into from

Conversation

jjgalvez
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

zzzeek commented May 25, 2024

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 {+

…of {} brackets. If it is a "normal" expression without contained {} then fall back to the usual regular expression. If the expression contains multiple {} then use the more complex regular expression.
@jjgalvez
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@jjgalvez jjgalvez closed this May 27, 2024
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.

2 participants