-
Notifications
You must be signed in to change notification settings - Fork 335
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
Additional percent-encode sets #837
base: main
Are you sure you want to change the base?
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.
These sets are duplicated across percent_encoding
and url/src/parser.rs
now.
Can you de-dupe?
Good point! Will do. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #837 +/- ##
==========================================
- Coverage 81.75% 81.42% -0.33%
==========================================
Files 20 20
Lines 3551 3516 -35
==========================================
- Hits 2903 2863 -40
- Misses 648 653 +5 ☔ View full report in Codecov by Sentry. |
deeeafc
to
7cf28a9
Compare
…t-url into addl-percent-encode-sets
Let me know if there's anything more I need to do in order to help get this in, thanks! |
Bump. Had to do some duplicate work in miniserve in lieu of this. I see in UPGRADING.md that these sets were previously included but have since been removed, but I could not find a justification. Does someone know why this decision was made? Regardless, I'm in favour of seeing these sets merged. |
Hi, I just got the PR updated and there seems to be a need for this work, can we please get this merged? I keep finding bits and pieces of it getting duplicated elsewhere now, too. Ping @lucacasonato |
Ping @joshka |
I've added some additional constants that should be useful for others.
I'd also recommend adding #721.
I've also corrected some docs generation warnings.