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

Use Addressable::URI instead of stdlib URI for normalization #1006

Merged
merged 1 commit into from
Apr 18, 2018

Conversation

casperisfine
Copy link

Alternative to #1005

Instead of trying to only escape the relevant fragments, this PR still applies escaping to all URLs, but does it with Addressable::URI, which is better at figuring out what needs to be escaped and what doesn't.

Example:

>> URI::Parser.new.escape(URI::Parser.new.escape('/foo bar'))
=> "/foo%2520bar"
>> Addressable::URI.parse(Addressable::URI.parse('/foo bar').normalize.to_s).normalize.to_s
=> "/foo%20bar"
>> Addressable::URI.parse('/foo[bar]').normalize.to_s
=> "/foo%5Bbar%5D"

It also have the advantage of using the same parser for every MRI versions which solves the parser discrepancies in #1005

@tarebyte what do you think?

@casperisfine
Copy link
Author

The 2.4.3 build ran into what look like a MRI bug. I can't trigger a retry but I think it should pass.

Copy link
Contributor

@kytrinyx kytrinyx 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 ⚡️

@tarebyte tarebyte merged commit 9799b12 into octokit:master Apr 18, 2018
@tarebyte
Copy link
Member

Thanks @casperisfine!

@casperisfine
Copy link
Author

Thanks to you!

@tarebyte
Copy link
Member

tarebyte commented May 8, 2018

This has been released as part of https:/octokit/octokit.rb/releases/tag/v4.9.0

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.

4 participants