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

feat(percent-encoding): add support for preserving characters when decoding #970

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ForsakenHarmony
Copy link

@ForsakenHarmony ForsakenHarmony commented Sep 19, 2024

This is useful to match the behavior of JavaScript's decodeURI for example.

I've also made the functions not take a static reference to the ASCII set for inline inversions.

And I've changed AsciiSet::EMPTY (added in #969) to be a reference to match the existing constants (I'm not sure if they need to be references, but I feel like it's better to have the same behavior everywhere).

@ForsakenHarmony ForsakenHarmony changed the title feat: add support for preserving characters when decoding feat(percent-encoding): add support for preserving characters when decoding Sep 19, 2024
@joshka
Copy link
Contributor

joshka commented Sep 20, 2024

Copied from #969, to make this the canonical place to discuss:


I wasn't 100% sure about where to put the constant. I went with EMPTY being a constant on the AsciiSet as empty seems like an inherent property of a type, but the other constants seem like usages of AsciiSet. I was 70/30% on this being right, so wouldn't object to this being changed to be consistent with the other constants.

The rationale for making the constants references rather than just values all seemed odd to me. What was that necessary for?

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