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 normalize_url option #111

Merged
merged 1 commit into from
Jan 15, 2015

Conversation

hokaccha
Copy link
Contributor

ref: #110

metainspector can't access This page.

Template:~ - Wikipedia

But, another HTTP Client can access it.

require 'metainspector'
require 'open-uri'
require 'httpclient'
require 'faraday'

url = 'http://ja.wikipedia.org/wiki/Template:%EF%BD%9E'

# metainspector
puts MetaInspector.new(url).response.status #=> 404

# open-uri
puts open(url).status #=> 200

# httpclient
puts HTTPClient.new.get(url).status #=> 200

# faraday
puts Faraday.new(url).get.status #=> 200

This issue is caused URL normalization. This patch allows you to disable URL normalization by option.

MetaInspector.new(url, normalize_url: false)

@jaimeiniesta
Copy link
Owner

Thanks @hokaccha, your PR is OK, but before we add this option let me ask at the Addressable project to see if this is a bug in normalization on their side. If not, I'll merge this.

@jaimeiniesta
Copy link
Owner

@hokaccha here's the issue I've opened upstream in case you want to participate in the conversation:

sporkmonger/addressable#182

@hokaccha
Copy link
Contributor Author

@jaimeiniesta OK, Thank you for your report!

jaimeiniesta added a commit that referenced this pull request Jan 15, 2015
@jaimeiniesta jaimeiniesta merged commit 55736e6 into jaimeiniesta:master Jan 15, 2015
@jaimeiniesta
Copy link
Owner

No news from the addressable team yet, so I've merged this to be able to skip normalization.

Thanks @hokaccha 👍

@jaimeiniesta
Copy link
Owner

I've released v4.1.0 which includes this patch.

@hokaccha hokaccha deleted the normalize_url_option branch January 15, 2015 12:56
@hokaccha
Copy link
Contributor Author

@jaimeiniesta Thanks 👍

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