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

Same URL in Unicode isn't equal to the same ASCII URL #94

Open
jage opened this issue Sep 14, 2016 · 7 comments
Open

Same URL in Unicode isn't equal to the same ASCII URL #94

jage opened this issue Sep 14, 2016 · 7 comments
Labels

Comments

@jage
Copy link
Contributor

jage commented Sep 14, 2016

This is the case since we compare with to_s (code).

[15] pry(main)> u1 = Twingly::URL.parse("https://www.foo.ایران.ir/bar")
=> #<Twingly::URL:0x3fc621e4ca10 https://www.foo.ایران.ir/bar>
[16] pry(main)> u2 = Twingly::URL.parse("https://www.foo.xn--mgba3a4f16a.ir/bar")
=> #<Twingly::URL:0x3fc621e00f34 https://www.foo.xn--mgba3a4f16a.ir/bar>
[17] pry(main)>
[18] pry(main)> u1 <=> u2
=> 1
[19] pry(main)> u1.to_s
=> "https://www.foo.ایران.ir/bar"
[20] pry(main)> u2.to_s
=> "https://www.foo.xn--mgba3a4f16a.ir/bar"
[21] pry(main)>

We might also consider using the normalized version when comparing (might not be what we want though).

@jage jage added the bug label Sep 14, 2016
@dentarg
Copy link
Collaborator

dentarg commented Sep 14, 2016

Yes, we need to normalize internally.

@dentarg
Copy link
Collaborator

dentarg commented Sep 14, 2016

$ bundle console
[1] pry(main)> u1 = Twingly::URL.parse("https://www.foo.ایران.ir/bar")
=> #<Twingly::URL:0x3fd236143104 https://www.foo.ایران.ir/bar>
[2] pry(main)> u2 = Twingly::URL.parse("https://www.foo.xn--mgba3a4f16a.ir/bar")
=> #<Twingly::URL:0x3fd2351830e8 https://www.foo.xn--mgba3a4f16a.ir/bar>
[3] pry(main)> u1 == u2
=> false

@jage
Copy link
Contributor Author

jage commented Sep 14, 2016

@dentarg we use Comparable, so this affects a few generated methods, and all will be fixed when <=> is fixed.

The Comparable mixin is used by classes whose objects may be ordered. The class must define the <=> operator, which compares the receiver against another object, returning -1, 0, or +1 depending on whether the receiver is less than, equal to, or greater than the other object. If the other object is not comparable then the <=> operator should return nil. Comparable uses <=> to implement the conventional comparison operators (<, <=, ==, >=, and >) and the method between?.

http://ruby-doc.org/core-2.3.0/Comparable.html

@dentarg
Copy link
Collaborator

dentarg commented Oct 13, 2016

We should also look over Hash Equality

$ irb
irb(main):001:0> require 'twingly/url'
=> true
irb(main):002:0> Twingly::URL::VERSION
=> "5.0.1"
irb(main):003:0> url = 'http://google.com'
=> "http://google.com"
irb(main):004:0> url1 = url2 = Twingly::URL.parse(url)
=> #<Twingly::URL:0x3fe0c1959fac http://google.com>
irb(main):005:0> { url1 => 'url1' }.has_key?(url2)
=> true

@dentarg
Copy link
Collaborator

dentarg commented Oct 13, 2016

Oops, should not have done url1 = url2 = Twingly::URL.parse(url). Here's what I mean

irb(main):009:0> url2 = Twingly::URL.parse(url)
=> #<Twingly::URL:0x3fe0c1910898 http://google.com>
irb(main):010:0> { url1 => 'url1' }.has_key?(url2)
=> false
irb(main):011:0> { url1 => 'url1' }.keys.include?(url2)
=> true

@dentarg
Copy link
Collaborator

dentarg commented Dec 7, 2018

Dumping related/interesting links: https://bugs.ruby-lang.org/issues/12852, https://url.spec.whatwg.org/

@jage
Copy link
Contributor Author

jage commented Feb 5, 2019

We should also look over Hash Equality

I think this was implemented in #129

$ bundle exec pry
[1] pry(main)> require_relative "lib/twingly/url"
=> true
[2] pry(main)> url = 'http://google.com'
=> "http://google.com"
[3] pry(main)> url1 = Twingly::URL.parse(url)
=> #<Twingly::URL:0x3fc3b6db63ac http://google.com>
[4] pry(main)> url2 = Twingly::URL.parse(url)
=> #<Twingly::URL:0x3fc3b6d939b0 http://google.com>
[5] pry(main)> { url1 => 'url1' }.has_key?(url2)
=> true
[6] pry(main)> { url1 => 'url1' }.keys.include?(url2)
=> true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants