Allow custom escape #1386
Allow custom escape #1386
Conversation
src/jinja2/filters.py
Outdated
| @@ -273,7 +316,11 @@ def do_xmlattr( | |||
| rv = " " + rv | |||
|
|
|||
| if eval_ctx.autoescape: | |||
| rv = Markup(rv) | |||
| # We don't assume that using this filter we will use a custom | |||
There was a problem hiding this comment.
| # We don't assume that using this filter we will use a custom | |
| # We don't assume that using this filter will use a custom |
Maybe this way? Sentence made no sense to me.
src/jinja2/nodes.py
Outdated
| # We need to keep Markup Class if existing as autoescape can be | ||
| # overwritten by {% autoescape %} environment. |
docs/api.rst
Outdated
| ) | ||
|
|
||
|
|
||
| Note that for `.world` files the `{{ var|e }}` and `{{ var | escape }}` |
There was a problem hiding this comment.
I think all this should use double backticks (assuming you want the equivalent to single-backtick highlighting on github)
docs/api.rst
Outdated
| template = env.get_template("message_to_the.world") | ||
| # the content of the template is simply assumed to by | ||
| """ | ||
| <h1>My Message to the world<h1> |
There was a problem hiding this comment.
| <h1>My Message to the world<h1> | |
| <h1>My Message to the world</h1> |
src/jinja2/ext.py
Outdated
| @@ -203,7 +201,6 @@ class InternationalizationExtension(Extension): | |||
|
|
|||
| tags = {"trans"} | |||
|
|
|||
| # TODO: the i18n extension is currently reevaluating values in a few | |||
There was a problem hiding this comment.
removing that line probably wasn't intended...
There was a problem hiding this comment.
Hi @CarliJoy,
I suggested some improvements regarding spelling and phrasing. Take my suggestions with a grain of salt as I am neither a Flask maintainer nor a native English speaker.
As I found many typos, I strongly suggest that you consider installing a spell checker for your IDE or editor.
If that is not possible, please give me a ping after your merged /considered my suggestions, then I will download your branch and review it locally - I just went through the PR in the GitHub UI.
That said - wow, what a massive PR - hats off!
src/jinja2/environment.py
Outdated
| each file type. | ||
|
|
||
| .. versionchanged:: 3.0 | ||
| if `autoescape` function returns not True or False but a Callable |
There was a problem hiding this comment.
| if `autoescape` function returns not True or False but a Callable | |
| if the `autoescape` function does not return True or False but a Callable |
|
Hi guys, thanks for your efforts, will work you comments in tomorrow or the days after tomorrow. So thanks for taking the effort in checking and correcting -> much appreciated! Also is the first time for me that an PR in github actually got reviewed, so I also need to get started on this slowly :) |
src/jinja2/utils.py
Outdated
| for key, func in sorted( | ||
| special_extensions.items(), key=lambda x: len(x[0]), reverse=True | ||
| ): |
There was a problem hiding this comment.
The commend above is overly verbose IMO. Something like "Lookup autoescape function using the longest matching suffix" explains it just as well.
That aside, do we really need this complexity? The only file type where double extensions are somewhat common are archives (.tar.{gz,xz,bz2}). So I'd rather just match using the actual extension (as determined by os.path.splitext) instaed of making this more complex than necessary.
There was a problem hiding this comment.
Well I can imagine end users that want a custom escape function for .special.html and also i.dont.like.spaces.but.points.html
There was a problem hiding this comment.
What would be the use case for this? I get .html vs .tex, but anything .html should probably use the standard HTML escaper...
There was a problem hiding this comment.
People could (arguably maybe misuse) use the escape function to escape all bad words like "fuck" from endusers.
Or people use a javascript framework that replaces a specific part of the HTML, so the end users should never be allow to write this special string.
IMO libraries should be enablers and if I see a potential bug/edge case I rather fix it right away as frustrate an end user/library user in the end.
There was a problem hiding this comment.
arguably maybe misuse
indeed, which is why I'm -1 on this
Or people use a javascript framework that replaces a specific part of the HTML, so the end users should never be allow to write this special string.
IMO libraries should be enablers and if I see a potential bug/edge case I rather fix it right away as frustrate an end user/library user in the end.
I get your point, but when people go into that kind of obscure territory they should probably provide their own select_autoescape implementation... In the end, libraries/framework should do their main jobs well, and if someone wants to do obscure things they can do that by subclassing/overriding things (or in this case providing their own autoescape function)
There was a problem hiding this comment.
Sure but honestly I don't know any better solution 💫.
Using splitext simply feels wrong for me:
from os.path import splitext
splitext('foobar.ver1.html')
('foobar.ver1', '.html')
splitext('foobar.gz.html')
('foobar.gz', '.html')
splitext('foobar.tar.gz')
('foobar.tar', '.gz')
So please provide a change request with your preferred solution . Already spend much more time on this PR then intended and really would not like to invest time implementing something that feels like implementing a 🐞 bug for me. 🧘
|
the moment when you worked 4hours to resolve a merge conflict and you realise that in the meanwhile the next PR was merged that causes the next merge conflict 😭 |
|
@ThiefMaster I fixed everything so far. Documentation is up to date, tests are as well. Currently no Merge Conflicts. Also because this change touches code on so many places, that other PR most likely will introduce other Merge Conflicts. |
Don't worry about that. If we believed this was something we don't want we'd have told you from the beginning. However, this is a large and complex PR, and while I will have another look, I don't think I'm going to be the one that will take the final decision on it and merge it; I'll leave that up to @davidism once he has the time for it. I wouldn't worry too much about merge conflicts - most of them can probably be resolved right before merging. |
|
@davidism once you check the code, have a lock especially on the async stuff. I have no experience with async coding what so ever, so my changes are more like an educated guess. |
|
@davidism can I support/help you in any way with the review? |
|
We won't have time to review something of this magnitude before release, so we're marking this for 3.1. I'm currently adding type annotations to the entire code base, so this is likely to need a rebase after that. Rebasing in general to squash commits would also be helpful, as 71 is too many to review. |
|
Sorry I am not yet that experienced with rebasing.
Would that work/help? Or what else do you mean? |
|
I think @davidism meant, that you should squash (https://levelup.gitconnected.com/how-to-squash-git-commits-9a095c1bc1fc) your 71 commits into one (and maybe force push the result to this branch). After @davidism applied the type annotations, and merged them into master/main, you can rebase onto master/main - but only this one commit (https://stackoverflow.com/questions/7929369) - which makes it much easier. Finally, force-push to this branch. So, it is neither required to create a new branch or a new PR. |
|
Hold off on squashing until I merge the type annotations. It might actually be easier to recreate after the fact instead of trying to resolve the merge conflicts, which will be significant. |
|
@davidism just tell me once you finished the merge of the type annotations... |
Thanks for the detailed answer |
|
They're merged |
|
@davidism Did the rebase and combination as requested. My suggestion would be to create a small PR that only provides the functions to get the (then only hardcoded) Markdown and escape functions. Is this understandable? |
|
We don't follow semver. I don't understand what the breaking change is, the only thing this PR should be doing is adding customization capabilities, not changing existing behavior. |
|
First of all. Sorry I missed that release. Big thing, updating all libraries at once. Congratulations 🎉
Braking change is for all creators of custom extensions and people that use Markdown directly in normal Python code to define a string as safe. |
|
That's not a breaking change though, that use case will continue to work, but they'll need to update if they want to take advantage / make correct use of customization that this enables. That's what a good changelog message and docs are for, so if you've addressed that there, that's good. |
|
@davidism I changed things as requested. If there are still any issues, let me know. |
CarliJoy
force-pushed
the
custom_escape
branch
2 times, most recently
from
0fc2b15 to
ee08594
Compare
|
@davidism and @ThiefMaster will this ever be merged? |
|
Yes, I do plan to eventually merge this. It is a huge set of changes, and I just have not been able to sit down and properly review it. |
|
@davidism just another ping. Anything I can do to support you on this MR? |
|
@davidism And here the yearly ping again 😉 . Anything I could do to finally finally get this merged? |
|
Happy 3rd birthday MR :-) @davidism I know this is a huge MR, due to the way how autoescape is handled currently. In the meanwhile I gained a lot more experience, also with async. I could review the MR again, rebase it and see if I am able to maybe reduce the number of changes lines. But I would only invest this time if there are resources on pallets side to review and merge the MR. Otherwise I will check in a year again. Feel free to reach out to me with tasks and questions related to this MR 🤗 |
Checklist:
CHANGES.rstsummarizing the change and linking to the issue... versionchanged::entries in any relevant code docs.pre-commithooks and fix any issues.pytestandtox, no tests failed.