search

keithmifsud / jekyll-target-blank

Automatically opens external links in a new browser for Jekyll Pages, Posts and Docs.
MIT License
105 stars 19 forks source link

Add support for Unicode links #46

Open mukrop opened 5 years ago

mukrop commented 5 years ago
mukrop commented 5 years ago

Sorry it took so long, my past two weeks were also rather busy.

keithmifsud commented 5 years ago

Hi @mukrop ,

Thank for sharing your work :)

The travis checks are failing can you look into why, please?

Also, thank you for the version number :)

mcrobs commented 4 years ago

The problem is that URI escape is deprecated, I suggest to use CGI.escape() instead. They do NOT behave in the same way, but in this particular case it does not matter how the URI is escaped, because this is for comparison only

lib/jekyll-target-blank.rb:196:21: W: Lint/UriEscapeUnescape: URI.escape method is obsolete and should not be used. Instead, use CGI.escape, URI.encode_www_form or URI.encode_www_form_component depending on your specific use case. 459 URI.parse(URI.escape(link)).host != URI.parse(URI.escape(@site_url)).host 460 ^^^^^^^^^^^^^^^^ 461lib/jekyll-target-blank.rb:196:57: W: Lint/UriEscapeUnescape: URI.escape method is obsolete and should not be used. Instead, use CGI.escape, URI.encode_www_form or URI.encode_www_form_component depending on your specific use case. 462 URI.parse(URI.escape(link)).host != URI.parse(URI.escape(@site_url)).host 463 ^^^^^^^^^^^^^^^^^^^^^

keithmifsud commented 4 years ago

The problem is that URI escape is deprecated, I suggest to use CGI.escape() instead. They do NOT behave in the same way, but in this particular case it does not matter how the URI is escaped, because this is for comparison only

lib/jekyll-target-blank.rb:196:21: W: Lint/UriEscapeUnescape: URI.escape method is obsolete and should not be used. Instead, use CGI.escape, URI.encode_www_form or URI.encode_www_form_component depending on your specific use case. 459 URI.parse(URI.escape(link)).host != URI.parse(URI.escape(@site_url)).host 460 ^^^^^^^^^^^^^^^^ 461lib/jekyll-target-blank.rb:196:57: W: Lint/UriEscapeUnescape: URI.escape method is obsolete and should not be used. Instead, use CGI.escape, URI.encode_www_form or URI.encode_www_form_component depending on your specific use case. 462 URI.parse(URI.escape(link)).host != URI.parse(URI.escape(@site_url)).host 463 ^^^^^^^^^^^^^^^^^^^^^

Thank you @mcrobs .. sorry for the cheeky request but is any chance you can update this PR by @mukrop to use the other parser, please?

If not I'll try to get to it asap but I'm rather busy and away from a Ruby IDE at this moment.

mukrop commented 4 years ago

I do apologize for my inactivity, but I'm rather busy now. I'd be glad if you managed to resolve this without me.

keithmifsud commented 4 years ago

I understand and thank you for all your help in this 😄

wbsmolen commented 4 years ago

@mukrop @keithmifsud polite bump :)

rafaelbiriba commented 4 years ago

Any news on this one?

mukrop commented 3 years ago

Hi. I've looked into this after a longer while. In the end, I used the Addressable class for URI parsing as

  1. URI is not capable to process non-ascii characters (see https://bugs.ruby-lang.org/issues/12852)
  2. URI.escape is deprecated
  3. Alternatives such as CGI.escape do a different thing (they escape the :// that separates the protocol and thus breaks the parsing).

The tests now pass but the build fails on not finding .rubocop_todo.yml (which I don't understand and seems unrelated).

jochenjonc commented 2 years ago

@mukrop can we borough a little bit of your time to see if #61 is the fix for #46? Maybe just approving 2 PRs and creating a release could help a lot of us.

mukrop commented 2 years ago

I'm off for the rest of the week but I believe I'll be able to find some time for this next week :-).

mukrop commented 2 years ago

Hi. I've tested the patch, but the issue is not fixed, the error seems the same.

/usr/lib/ruby/2.7.0/uri/rfc3986_parser.rb:21:in `split': URI must be ascii only (URI::InvalidURIError)
jochenjonc commented 2 years ago

Hi. I've tested the patch, but the issue is not fixed, the error seems the same.

/usr/lib/ruby/2.7.0/uri/rfc3986_parser.rb:21:in `split': URI must be ascii only (URI::InvalidURIError)

Could it be that the code or test isn't using the Addressable implementation of URI, but still the standard on of Ruby?

mukrop commented 2 years ago

Aah, sorry @jochenjonc, I misunderstood what I was supposed to check. I did it properly now and, indeed, #61 fixes the thing!

For testing purposes, I've added an extra commit to this PR replicating #61. For the sake of a clean codebase, I presume you want to merge #61 first and then merge this onto it without the replicating commit?

jochenjonc commented 2 years ago

@mukrop you can choose how to merge everything. None of the PRs are mine. 😉

mukrop commented 2 years ago

Oh, and you are not the repository owner either, I thought you were :-).

jochenjonc commented 2 years ago

Funny, I thought you where the repo owner. 🤔

jochenjonc commented 2 years ago

@keithmifsud are you able to merge this PR and create a new release?

keithmifsud commented 2 years ago

Hi @jochenjonc ..I'll merge it asap. I need to remove the Travis CI check first but they seem to have changed the access. I'll get back to you asap.

The PR looks good. Thank you :smile:

jochenjonc commented 2 years ago

@keithmifsud any luck on removing Travis CI check?