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

Do not hardcode locale unless certainly necessary #6791

Merged
merged 1 commit into from Feb 25, 2018

Conversation

ashmaroli
Copy link
Member

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 a user February 20, 2018 07:19
Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

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

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

@pathawks
Copy link
Member

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

@DirtyF
Copy link
Member

DirtyF commented Feb 20, 2018

/cc @alextsui05

@ashmaroli
Copy link
Member Author

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.

Copy link

@sara81 sara81 left a comment

Choose a reason for hiding this comment

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

(removed by maintainer)

@pathawks
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member

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

@ashmaroli
Copy link
Member Author

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?
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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]

Copy link
Member

Choose a reason for hiding this comment

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

👍 Thanks for checking!

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@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 mentioned this pull request Feb 25, 2018
2 tasks
@ghost
Copy link

ghost commented Feb 25, 2018

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 017f032 into jekyll:master Feb 25, 2018
jekyllbot added a commit that referenced this pull request Feb 25, 2018
ghost pushed a commit that referenced this pull request Feb 25, 2018
ghost pushed a commit that referenced this pull request Feb 25, 2018
@ghost ghost removed this from the v3.8.0 milestone Feb 25, 2018
@ashmaroli ashmaroli deleted the drop-hardcoded-locale branch February 25, 2018 12:08
@jekyll jekyll locked and limited conversation to collaborators Jul 12, 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.

Incompatibility between Jekyll 3.7 and I18n filter plugin
8 participants