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

Add support for Unicode links #8

Closed
wants to merge 1 commit into from
Closed

Add support for Unicode links #8

wants to merge 1 commit into from

Conversation

mukrop
Copy link

@mukrop mukrop commented Sep 13, 2019

* In function 'external?', URI is now escaped before split.
* This adheres to the recommendation in the URI docs: https://www.rubydoc.info/stdlib/uri/URI.parse
* Using CGI.escape instead of URI.escape as the latter is marked obsolete.
* Resolves keithmifsud#33
@mukrop mukrop mentioned this pull request Sep 13, 2019
@mukrop
Copy link
Author

mukrop commented Sep 13, 2019

Details described in #9.

@kenchan0130
Copy link
Owner

@mukrop
Thank you for your contribution.

Looks good.
If you can, please add test about included multibyte chars link.

@mukrop
Copy link
Author

mukrop commented Oct 6, 2019

Unfortunately, no :-/.
I'm not a Ruby developer and after attempting I did not even find the tests (and am unsure how to test it in principle). Furthermore, I cannot get the CI passing (even though it compiled OK in my local project).

Feel free to close this pull request as it's probably not merge-quality. I just wanted to add what I found if it was helpful for others (even though I was unable to make a proper functional pull request).

@keithmifsud
Copy link

Hi @mukrop, can you kindly submit the PR to the original repo? I have some time this week to look into :)

@mukrop
Copy link
Author

mukrop commented Oct 27, 2019

I'll resubmit the PR to the original repo. Closing this one.

@mukrop mukrop closed this Oct 27, 2019
@kenchan0130
Copy link
Owner

I'm sorry for your inconvenience but thank you for your cooperation.

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

Successfully merging this pull request may close these issues.

Error: URI must be ascii only
3 participants