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

Add percent encoding function #2052

Merged
merged 5 commits into from
May 19, 2024
Merged

Add percent encoding function #2052

merged 5 commits into from
May 19, 2024

Conversation

laniakea64
Copy link
Contributor

#1982

This function encodes more characters than Javascript encodeURIComponent because the Examples section of that page mentions some cases that require additional percent-encoding, and in my experience extra percent-encoding like this doesn't hurt anything.

Not happy about the static in light of #2049 (comment) but don't know any way around it ☹️

@casey
Copy link
Owner

casey commented May 18, 2024

My issue with static is just that it requires navigating to the definition of the static to find out what it actually contains, not that it's static per se. I moved the static inside the function, so it's not an issue here.

Regarding which characters to actually percent encode, I think it might be useful to provide functions which exactly match encodeURI and encodeURIComponent, that way, users aren't surprised by which characters are encoded.

We could also then later add a function which percent encodes even more things, if that winds up being useful. I'd rather percent encode more things based on specific feedback and concrete use-cases.

What do you think?

@laniakea64
Copy link
Contributor Author

Changed to match encodeURIComponent behavior. Per #1982 (comment) encodeURIComponent would fill the original request. And should also work for how I might use it.

I don't recall ever using encodeURI.

@casey casey enabled auto-merge (squash) May 19, 2024 00:35
@casey
Copy link
Owner

casey commented May 19, 2024

LGTM! I renamed it to encode_uri_component, so that we have the ability to add a more flexible percent_encode function later on, perhaps one which takes as input a string containing a custom list of characters to escape.

@casey casey merged commit 4961f49 into casey:master May 19, 2024
5 checks passed
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