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

Add support for non-ascii URIs #197

Merged
merged 1 commit into from
Apr 1, 2015
Merged

Add support for non-ascii URIs #197

merged 1 commit into from
Apr 1, 2015

Conversation

ixti
Copy link
Member

@ixti ixti commented Mar 31, 2015

Resolves #196

@ixti ixti added this to the v0.9 milestone Mar 31, 2015
zanker
zanker reviewed Mar 31, 2015
View reviewed changes
lib/http/uri_normalizer.rb
#
# @param [#to_s] uri
# @return [::URI]
def normalize_uri(uri)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just use addressable? I see in the comments on the issue it's mentioned, but I don't see a problem with adding a dependency on something that supports URIs more closely to the RFC. This workaround seems very ugly :(

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with a hard dependency on addressable

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I'm 100% agree that this hack is ugly as .
And I'm OK with Addressable, but not as hard dependency.

Copy link
Member

Choose a reason for hiding this comment

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

Why not?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to support non-ASCII, we should fully support them imo. Eg, http://🚚.la doesn't work for URI, while it does for Addressable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Although Addressable is absolutely awesome gem, it's about 1.7x slower on parsing URIs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyway, I guess I'm just trying to reject any changes today.. :D Need to go sleep! :D Will replace this patch with addressable based solution ;))

@ixti ixti force-pushed the improve/uri branch 5 times, most recently from 88d7cf9 to 48e7437 Compare March 31, 2015 02:20
@ixti
Copy link
Member Author

ixti commented Mar 31, 2015

@httprb/owners thoughts? I would sneak this into 0.8.0 release.

@tarcieri
Copy link
Member

This seems ok to me

@digitalextremist
Copy link

The only thing I could see adding is a test with non-ascii characters in a URI.

@sferik
Copy link
Contributor

sferik commented Mar 31, 2015

👍 I think Addressable::URI trying to be a drop-in replacement for the URI standard library (similar to how FasterCSV replaced CSV in the standard library).

I made the decision to replace URI with Addressable::URI in the twitter gem over a year ago prompted by a similar issue.

@sferik
Copy link
Contributor

sferik commented Mar 31, 2015

FWIW, the relevant specs in the twitter gem look like this: https://github.com/sferik/twitter/blob/6750ece8a9c44482c073958814810b6cf27e1d1a/spec/twitter/tweet_spec.rb#L354-L368.

@ixti
Copy link
Member Author

ixti commented Mar 31, 2015

@digitalextremist Isn't this is enough?

https://github.com/httprb/http.rb/pull/197/files#diff-3ef70b884bdcb4e458a8230f6a2b4fb8R81
https://github.com/httprb/http.rb/pull/197/files#diff-3ef70b884bdcb4e458a8230f6a2b4fb8R171

I'm ok to add more examples though.

@ixti
Copy link
Member Author

ixti commented Mar 31, 2015

We can add following URLs (real ones) to tests if you think that worth it:

Both of above will lead to this page :D

@digitalextremist
Copy link

@ixti I think it's worth it!

@ixti
Copy link
Member Author

ixti commented Mar 31, 2015

@digitalextremist OK. Will add later today.

@ixti ixti self-assigned this Mar 31, 2015
@ixti ixti modified the milestones: v0.8, v0.9 Mar 31, 2015
@digitalextremist
Copy link

Extra points for incorporating the 無 symbol @ixti :)

@ixti
Copy link
Member Author

ixti commented Mar 31, 2015

@digitalextremist 👍

zanker
zanker reviewed Mar 31, 2015
View reviewed changes
lib/http/client.rb
base.query = nil
base.path = ""
base.to_s
uri.omit(:query, :path).to_s
Copy link
Contributor

Choose a reason for hiding this comment

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

I already like Addressable more than URI!

Copy link
Member Author

Choose a reason for hiding this comment

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

:D

@ixti
Copy link
Member Author

ixti commented Apr 1, 2015

That was a good idea to test behavior on a real connection...
/me still tries to debug -- connection just hangs...

@ixti
Copy link
Member Author

ixti commented Apr 1, 2015

Ouch. found the issue :D

ixti added a commit that referenced this pull request Apr 1, 2015
@ixti
Merge pull request #197 from httprb/improve/uri
9496fc6 
Add support for non-ascii URIs
@ixti ixti merged commit 9496fc6 into master Apr 1, 2015
@ixti ixti deleted the improve/uri branch April 1, 2015 17:56
@gmile
Copy link

gmile commented Jan 11, 2016

TIL addressable gem consumes quite a bit of memory: zquestz/omniauth-google-oauth2#193 (comment). Would a PR to move back to stdlib's very own URI implementation be feasible?

@zanker
Copy link
Contributor

zanker commented Jan 11, 2016

Ackkkk :(.

I'm sure it's feasible, although I'm not sure how much it's reinventing the wheel ultimately.

@ixti
Copy link
Member Author

ixti commented Jan 11, 2016

Moving to pure stdlib will require fair amount of work. But definitely possible.

@ixti ixti mentioned this pull request Jan 19, 2016
Closed
@tarcieri tarcieri mentioned this pull request Mar 19, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ixti ixti

Labels
None yet
Projects
None yet
Milestone
v0.8
Development

Successfully merging this pull request may close these issues.

Dealing with non-ascii URIs (IRIs) in redirects
6 participants
@ixti @tarcieri @digitalextremist @sferik @gmile @zanker