Drop libidn #102
Drop libidn #102
Conversation
Instead, we use the pure Ruby IDNA solution in addressable. Close #66.
More information at https://curl.haxx.se/docs/adv_20161102K.html. Related to #101.
| @@ -102,6 +114,16 @@ def valid_urls | |||
| end | |||
| end | |||
|
|
|||
| context "IDNA 2003 vs IDNA 2008" do | |||
There was a problem hiding this comment.
context and its does not really follow suit with rest of the spec file. How about something like:
context "when given IDN URL containing ß" do # could generalize but it's early in the morning and brain is slow
...
it "should conform to the IDNA 2008 standard" do
...
end
endShould also be placed under the describe for #normalized, see for instance
twingly-url/spec/lib/twingly/url_spec.rb
Line 388 in 77631f7
There was a problem hiding this comment.
Actually, I thought it made sense to introduce a new "top level" context "IDNA 2003 vs IDNA 2008", considering we want to do things like #71. That I'm using normalized here is just me "cheating"... :)
There was a problem hiding this comment.
I know the wording doesn't follow what we have... it is hard writing these things. In the code, you have "it", but in the spec output, you don't have "it"... so should you take it into consideration or not?
Now I just used context as a "title" and then added an sentence that would read well (IMHO) in the spec output:
IDNA 2003 vs IDNA 2008
if you normalize a URL with the domain "straße.de"
the domain of the normalized URL should be "xn--strae-oqa.de"
There was a problem hiding this comment.
I know the wording doesn't follow what we have... it is hard writing these things. In the code, you have "it", but in the spec output, you don't have "it"... so should you take it into consideration or not?
You should add the it yourself when reading, it seems to be what RSpec does in their examples, check for instance:
https://www.relishapp.com/rspec/rspec-core/v/3-5/docs/example-groups/basic-structure-describe-it where you have it "does something" and https://www.relishapp.com/rspec/rspec-core/v/3-5/docs/example-groups/shared-examples where you can find it "should return...."
There was a problem hiding this comment.
considering we want to do things like #71. That I'm using normalized here is just me "cheating"... :)
I don't think it's cheating (as it seems like now, we need to #normalizeto trigger this, if I am not mistaken) I believe it should be put under that description. Easier to find and it's what we do currently for the most parts.
I think it's hard to reason about #71 at this stage as it makes everything more complex, let's take that discussion when we get there 😄
There was a problem hiding this comment.
Now I just used context as a "title" and then added an sentence that would read well (IMHO) in the spec output:
What makes sense to me, when it comes to context, is that you are describing a setting. It's not obvious in a library like this, so I agree it's hard too. It would be more obvious if we dealt with higher level stuff like "when logged in" vs "when not logged in".
There was a problem hiding this comment.
BTW, once upon a time I read http://betterspecs.org/ to try to figure out how to use contexts, describes and its. (they seem to dislike usage of the word should though, I must have forgotten that)
|
Could cleanup Ansible too: https://github.com/twingly/ansible/commit/c8d999f8464fc7a81a51d18d54c690069a7719fc. I don't think we should uninstall though, I am pretty sure other things depend on it 😄 |
|
Re: ansible, yes, we should clean up, but we can discuss that over there as it's really not that related to the gem itself |
Of course, it was just a reminder. |
Should we enable Travis CI tests for JRuby then? |
Good idea, will do |
9.0.5.0 because that's what we currently use, and 9.1.5.0 which is the latest release.
| # Long names like this will probably not resolv, see comments at | ||
| # https://github.com/twingly/twingly-url/issues/66#issuecomment-246349029 | ||
| # but for now as addressable considers them valid, we do the same. | ||
| "http://AcinusFallumTrompetumNullunCreditumVisumEstAtCuadLongumEtCefallumEst.com", |
There was a problem hiding this comment.
This can jump back up again once Addressable 2.5.0 is released with sporkmonger/addressable@c73810f
There was a problem hiding this comment.
Oh, it has been released, https://rubygems.org/gems/addressable/versions/2.5.0
There was a problem hiding this comment.
This can jump back up again once Addressable 2.5.0 is released with sporkmonger/addressable@c73810f
Nope, I got that wrong too. With Addressable 2.5.0 and libidn, the URL is now considered valid.
Instead, we use the pure Ruby IDNA solution in addressable.
Close #66.