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

Projects
None yet
3 participants
@pathawks
Member

pathawks commented Sep 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 24, 2016

Member

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?

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')

This comment has been minimized.

@pathawks

pathawks Sep 24, 2016

Member

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

@pathawks

pathawks Sep 24, 2016

Member

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

This comment has been minimized.

@parkr

parkr Sep 24, 2016

Member

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.

@parkr

parkr Sep 24, 2016

Member

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.

This comment has been minimized.

@pathawks

pathawks Sep 24, 2016

Member

Ok, that makes sense.

@pathawks

pathawks Sep 24, 2016

Member

Ok, that makes sense.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Sep 24, 2016

Member

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.

Member

pathawks commented Sep 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Sep 24, 2016

Member

Travis likes it!

Member

pathawks commented Sep 24, 2016

Travis likes it!

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 28, 2016

Member

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

Member

parkr commented Sep 28, 2016

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

@parkr

parkr approved these changes Sep 28, 2016

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

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 29, 2016

Member

@jekyllbot: merge +bug

Member

parkr commented Sep 29, 2016

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 553fae8 into master Sep 29, 2016

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot added bug fix labels Sep 29, 2016

@jekyllbot jekyllbot deleted the pr/i18n-url-filters branch Sep 29, 2016

jekyllbot added a commit that referenced this pull request Sep 29, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment