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

Suggest using addressable #22

Closed
linyaoli opened this issue Sep 12, 2018 · 2 comments
Closed

Suggest using addressable #22

linyaoli opened this issue Sep 12, 2018 · 2 comments

Comments

@linyaoli
Copy link
Contributor

linyaoli commented Sep 12, 2018

Hi,

Down::InvalidUrl
/usr/local/bundle/gems/down-4.5.0/lib/down/net_http.rb:236:in `rescue in ensure_uri'
/usr/local/bundle/gems/down-4.5.0/lib/down/net_http.rb:231:in `ensure_uri'
/usr/local/bundle/gems/down-4.5.0/lib/down/net_http.rb:65:in `download'
/usr/local/bundle/gems/down-4.5.0/lib/down/backend.rb:13:in `download'

the standard URI lib used by down cannot parse URLs with some bizarre characters, such as [.
Please consider to use https://github.com/sporkmonger/addressable, if you don't mind I can also make a PR for it.

Thanks in advance.

@janko
Copy link
Owner

janko commented Sep 20, 2018

Hey @linyaoli, that is a known issue, it's definitely a limitation of the built-in URI standard library. So far I was reluctant to use Addressable, as I go the impression that it was a relatively heavy gem, and the goal of down is to be lightweight.

However, for reliability I think the ends justify the means, so I've decided that I'm ok with it being added after all.

I think we can do it in a way that we call Addressable::URI#normalize! and pass the normalized URI further.

require "addressable/uri"

url = Addressable::URI.parse(url).normalize!.to_s
# ...

Feel free to open a PR for this.

@janko
Copy link
Owner

janko commented Sep 28, 2018

Implemented in 1764248.

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

No branches or pull requests

2 participants