-
Notifications
You must be signed in to change notification settings - Fork 12
Implement upper filter #81
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really good start thanks!
Unfortunately, upper
is a bit weird so this will need some updating to handle that.
src/render/filters.rs
Outdated
Some(content) => Some( | ||
content | ||
.resolve_string(context)? | ||
.map_content(|content| Cow::Owned(content.to_uppercase())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upper
is a bit unusual because it's not considered HTML-safe upstream in Django.
To handle this correctly here, we'll need to replace .map_content
with something like:
.map_content(|content| Cow::Owned(content.to_uppercase())), | |
.map(|content| content.content().to_uppercase()) | |
.map(|content| Cow::Owned(Content::String(content))), |
We'll also need a test to demonstrate we follow Django's behaviour here. (I'd add it in the Python tests.) This test should include a html tag which will become html-escaped as well as uppercased in the expected output.
For more information about this quirk, see my blog post or the ticket and Django PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Django case is really interesting.
I've been trying to use upper filter for HTML text in Django, and I'm not sure about the proposed solution. Django uses uppercase to encode text before encoding HTML characters. In this case, the resolve_string()
method will encode HTML characters; I always have to call resolve_string()
before calling to_uppercase()
in the code. So the encoded HTML characters will be capitalized, which Django doesn't do.
I added test cases, especially the last one which I feel is not solvable without change resolve_string
for allowing to return a HtmlUnsafe
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very good point - I think this is essentially a bug in the existing Rust implementation that's been exposed by your work here.
I've had a play around with your idea of introducing a HtmlUnsafe
variant to the Content
and ContentString
enums and I've got the tests passing. 🚀
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #81 +/- ##
==========================================
+ Coverage 98.73% 98.78% +0.04%
==========================================
Files 47 48 +1
Lines 7832 7958 +126
Branches 2811 2819 +8
==========================================
+ Hits 7733 7861 +128
Misses 61 61
+ Partials 38 36 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I had hoped we only needed two variants, but I now think it's conceptually simpler to have `String` for contexts where escaping behaviour is irrelevant and `HtmlSafe` and `HtmlUnsafe` where it matters.
This reduces some boilerplate when it doesn't matter which exact `ContentString` variant is present.
Thanks for making such a great start with this @LucasGrugru! I expect some of the other filters will be more straightforward to implement. |
Hello,
I find the project really interesting. I propose a very small contribution to add the uppercase filter.
I'm not a rust expert so feel free to tell me if anything needs to be changed.