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

Do not hardcode locale unless certainly necessary #6791

Merged
merged 1 commit into from Feb 25, 2018

Conversation

Projects
None yet
9 participants
@ashmaroli
Member

ashmaroli commented Feb 20, 2018

Fixes #6669
We shouldn't be hardcoding locales unnecessarily

@ashmaroli ashmaroli added this to the v3.8.0 milestone Feb 20, 2018

@ashmaroli ashmaroli requested review from mattr- and oe Feb 20, 2018

@DirtyF

DirtyF approved these changes Feb 20, 2018

As a non-english native, I can only approve this 😄

@oe

oe approved these changes Feb 20, 2018

@pathawks

This comment has been minimized.

Member

pathawks commented Feb 20, 2018

This was added in #6509. Was it necessary for something, or just boilerplate that got included?

@DirtyF

This comment has been minimized.

Member

DirtyF commented Feb 20, 2018

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Feb 20, 2018

Was it necessary for something, or just boilerplate that got included?

It's required.
For some reason, I18n raises an error if we do not pass :en as a locale in the event there are no locales defined..
Its probably a bug in I18n or in the implementation at our end, but as of this writing, its required.

@sara81

(removed by maintainer)

@pathawks

This comment has been minimized.

Member

pathawks commented Feb 20, 2018

For some reason, I18n raises an error if we do not pass :en as a locale in the event there are no locales defined..

So removing this default could lead to problems if plugins expect it to be there?
4.0?

@alextsui05

This comment has been minimized.

Contributor

alextsui05 commented Feb 20, 2018

Sorry, I wrote that line when adding latin mode and the situation is as ashmaroli says. I don't think there's any particular need other than to clear the error, and this PR looks good to limit setting it only where it's necessary.

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Feb 21, 2018

So removing this default could lead to problems if plugins expect it to be there?
4.0?

@pathawks Since this default was just added in 3.7.0, its safe to assume that not many plugins would have been written to use the default.
While technically, it can be considered a breaking change; though, for practical purposes, this is actually just a bug-fix that should be shipped a.s.a.p.

Ideally, we'd release this as a backport in 3.7.3 but since the maintainers are busy with other stuff, I guess this will just ship with 3.8.0..

@pathawks

This comment has been minimized.

Member

pathawks commented Feb 21, 2018

If there were enough bug fixes to merit a 3.7.3, I could probably do that later this week.

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Feb 21, 2018

If there were enough bug fixes to merit

I believe all other reported bugs have been fixed as part of 3.7.2
Besides, there has been at least two backport releases with just one bug-fix in the past..

@@ -203,7 +203,10 @@ def slugify(string, mode: nil, cased: false)
end
# Drop accent marks from latin characters. Everything else turns to ?
string = ::I18n.transliterate(string) if mode == "latin"
if mode == "latin"
I18n.config.available_locales = :en if I18n.config.available_locales.empty?

This comment has been minimized.

@Crunch09

Crunch09 Feb 22, 2018

Member

Should we set available_locales to an array ([:en]) instead?

This comment has been minimized.

@ashmaroli

ashmaroli Feb 22, 2018

Member

I don't think its necessary.
If you were to add a print I18n.config.available_locales immediately after this line, the output would return [:en]

This comment has been minimized.

@Crunch09

Crunch09 Feb 22, 2018

Member

👍 Thanks for checking!

This comment has been minimized.

@milosgajdos83

milosgajdos83 Mar 27, 2018

This code only gets run if mode is set to latin. If it's not you break everyone else's code by removing I18n.config.available_locales = :en from jekyll module which is exactly what happened to me.

This comment has been minimized.

@ashmaroli

ashmaroli Mar 27, 2018

Member

@milosgajdos83 I18n.config.available_locales = :en was specifically added to the Jekyll module for the sole use in the slugify filter.
Please open a new separate issue for better tracking..

@ashmaroli ashmaroli referenced this pull request Feb 25, 2018

Merged

Draft a release post for v3.7.3 #6803

2 of 2 tasks complete
@oe

This comment has been minimized.

Member

oe commented Feb 25, 2018

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 017f032 into jekyll:master Feb 25, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot added bug fix labels Feb 25, 2018

oe added a commit that referenced this pull request Feb 25, 2018

oe added a commit that referenced this pull request Feb 25, 2018

@oe oe removed this from the v3.8.0 milestone Feb 25, 2018

@ashmaroli ashmaroli deleted the ashmaroli:drop-hardcoded-locale branch Feb 25, 2018

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