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

[#128] Make case comparisons consistent between various locale settin… #255

Merged
merged 1 commit into from
Jul 30, 2016

Conversation

monkpow
Copy link

@monkpow monkpow commented Jul 20, 2016

…g methods (e.g. cookies, headers, query).

… setting methods (e.g. cookies, headers, query).
@monkpow
Copy link
Author

monkpow commented Jul 20, 2016

In the existing code line, the behavior is a bit different when using queryParameters to set locale in the case of a language code like en-US.

if setting via:
cookie => en-US
header => en-US
queryParameter => en-us

After reading it a few times, I suspect this is a bug, but have wrapped it in a config value just in case some current users rely on this behavior.

@@ -2,7 +2,7 @@ var i18n = require('../i18n'),
should = require("should"),
path = require("path");

describe('Locale switching should work queryParameter', function() {
describe('Locale switching should work when set via cookie', function() {
Copy link
Author

Choose a reason for hiding this comment

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

Not really related to this change set, but seemed polite to clean this up.

Copy link
Owner

Choose a reason for hiding this comment

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

yap I've seen this... thanks again - I bet there are still some more typos in readme, comments & tests - 6 years of part time maintenance...

@mashpie
Copy link
Owner

mashpie commented Jul 20, 2016

Thank you sir! It'd be best to handle those consistent internally. Refactoring such is still on my backlog, so your help catching up is much appreciated. I will review some extra changes on weekend to merge and publish.

@monkpow
Copy link
Author

monkpow commented Jul 20, 2016

Ok thanks, let me know if you'd prefer I pull out the config switch, and just fix it as a bug.

@mashpie
Copy link
Owner

mashpie commented Jul 20, 2016

the switch is fine - I am a big fan of feature switches and legacy switches could help with issues until next major release.

@coveralls
Copy link

coveralls commented Jul 20, 2016

Coverage Status

Coverage increased (+0.01%) to 97.912% when pulling 7e1c9e1 on monkpow:issue_128 into a74afa5 on mashpie:master.

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.

3 participants