-
Notifications
You must be signed in to change notification settings - Fork 1
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
Apply locale ID mappings consistently #16
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a small tidy up.
LGTM otherwise
Checked out the branch locally and ran it. First time it clashed with existing matching job names (not sure if that's something being tweaked on a separate branch?)
But after clearing down those jobs on the Smartling side, sync up to Smartling seemed fine:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worksforme!
Thanks both. I'll tidy up those unused imports and add a sprinkling of unit tests for the util functions and then get this merged. |
…skip over any reformatting if set to False
…of-locales Add option to disable case-changing of language codes
@bcdickinson Apols, I forgot I had targetted my |
Not to worry, I'll review those changes here, there were a couple of bits I wanted to mention. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few cosmetic tweaks needed.
Kudos for the smartling settings fixture!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely lovely lovely. Thanks for this, Ben!
PS: I agree with all of Dan's tweaks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with that small typo fix.
Great work, @bcdickinson
Fixes #15
@stevejalim take this with a pinch of salt, I have given it as thorough a shakedown as it needs, but I think this should fix the issue you showed me earlier. If you're able to give it a spin and let me know if it fixes your problems, that'd be great.