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

Fix handling of non-ASCII characters in new *_url filters #5410

Merged
merged 2 commits into from
Sep 29, 2016

Conversation

pathawks
Copy link
Member

How/where should normalization be handled? I would love if the filters could also be responsible for normalization.

@pathawks pathawks added the bug label Sep 24, 2016
@pathawks pathawks added this to the 3.3 milestone Sep 24, 2016
@parkr
Copy link
Member

parkr commented Sep 24, 2016

Punycode, you are the worst. Encoding URL's is notorious. I think this could be a fix for after 3.3 just because it's so gnarly.

Does Ruby stdlib have anything for this?

@@ -38,4 +38,5 @@ Gem::Specification.new do |s|
s.add_runtime_dependency('jekyll-sass-converter', '~> 1.0')
s.add_runtime_dependency('jekyll-watch', '~> 1.1')
s.add_runtime_dependency("pathutil", "~> 0.9")
s.add_runtime_dependency('addressable', '~> 2.4')
Copy link
Member Author

Choose a reason for hiding this comment

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

Addressable was already being installed implicitly, so this is not actually adding a new dependency.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't look like it's a dependency of jekyll, but it a dependency of jekyll-sitemap per a Gemfile.lock of mine using both. So it is technically not used yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, that makes sense.

@pathawks
Copy link
Member Author

I think this could be a fix for after 3.3 just because it's so gnarly.

Oh please no. I feel like plugins won't be able to rely on this filter until this is resolved.

Does Ruby stdlib have anything for this?

I don't think so, and that's why Addressable exists. It was already an implicit dependency, so I don't see a problem using it here.

@pathawks
Copy link
Member Author

Travis likes it! ✅

@parkr
Copy link
Member

parkr commented Sep 28, 2016

I'm OK with this. @jekyll/stability, any thoughts?

@parkr parkr changed the title New URL filters choke when URL contains non-ASCII characters Fix handling of non-ASCII characters in new *_url filters Sep 29, 2016
@parkr
Copy link
Member

parkr commented Sep 29, 2016

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 553fae8 into master Sep 29, 2016
@jekyllbot jekyllbot deleted the pr/i18n-url-filters branch September 29, 2016 20:27
jekyllbot added a commit that referenced this pull request Sep 29, 2016
@jekyll jekyll locked and limited conversation to collaborators Jul 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants