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.

Sign up for GitHub

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

Jump to bottom

Remove double decode/encode URI #180

Merged
merged 3 commits into from
Jan 6, 2025
Merged

Remove double decode/encode URI #180

merged 3 commits into from
Jan 6, 2025

Conversation

tsnobip
Copy link
Contributor

@tsnobip tsnobip commented Jan 1, 2025 •
edited
Loading

to rebase on main once #179 is merged

@tsnobip tsnobip marked this pull request as draft January 1, 2025 17:01
@tsnobip tsnobip mentioned this pull request Jan 1, 2025
Merged
@tsnobip tsnobip marked this pull request as ready for review January 1, 2025 17:39
@zth zth self-requested a review January 2, 2025 18:23
zth
zth approved these changes Jan 2, 2025
View reviewed changes
Copy link
Owner

@zth zth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me!

At one point we should probably move to using URLSearchParams so these things are handled automatically for us. But, I made a mistake when doing my own query params module here by implementing arrays as comma separated values rather than repeated assignments to the same name. Not sure there's a good way to unwind from that.

@tsnobip
Copy link
Contributor Author

tsnobip commented Jan 3, 2025

@zth would moving to URLSearchParams be a breaking change? I think it'd be the right time to take care of it. Most of my current PRs are actually cleanup more than new features.

@zth
Copy link
Owner

zth commented Jan 3, 2025

@zth would moving to URLSearchParams be a breaking change? I think it'd be the right time to take care of it. Most of my current PRs are actually cleanup more than new features.

Just how arrays are handled is the question, as mentioned above.

@tsnobip tsnobip force-pushed the remove-double-decodeuri branch 3 times, most recently from 38128fd to 852581a Compare January 6, 2025 12:17
@tsnobip tsnobip force-pushed the remove-double-decodeuri branch from 852581a to 7741e69 Compare January 6, 2025 12:21
@zth
Copy link
Owner

zth commented Jan 6, 2025

@tsnobip ready to go?

@tsnobip
Copy link
Contributor Author

tsnobip commented Jan 6, 2025

@tsnobip ready to go?

yes ready!

@zth zth merged commit ea3ae28 into zth:main Jan 6, 2025
2 checks passed
@tsnobip tsnobip deleted the remove-double-decodeuri branch January 6, 2025 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zth zth zth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tsnobip @zth