Update translations again by zachmargolis · Pull Request #1531 · 18F/identity-idp

zachmargolis · 2017-07-06T17:30:48Z

A few notes:

our vendor is giving us poorly formed XML so I patched the converter script to accept this
to try to keep things consitent, I ran i18n-tasks normalize after importing strings, which created a lot of noise in our en.yml and fr.yml files --- sorry for the diff noise but hopefully this should minimize noise in future PRs

zachmargolis · 2017-07-07T13:55:47Z

Update: I fixed some missing keys, I also removed the commit where I used Nokogiri to clean up the incoming XML, we got better results from our vendor.

zachmargolis · 2017-07-07T14:07:59Z

Also pushed another commit renaming an i18n key to be friendlier for future translation exports

monfresh · 2017-07-07T19:29:45Z

config/locales/devise/en.yml

It looks like this introduced a bunch of additional spaces before the end quote. I see several instances where this happened in this PR. Was that intentional?

What was the reason for replacing the > convention with quoted strings?

This is an artifact of i18n-tasks normalize and our use of the > when wrapping lines. The newlines were already there, this is just making them more explicit:

require 'yaml' => true YAML.load <<-STR key: key: > some string that we wrap other: thing STR => {"key"=>{"key"=>"some string that we wrap\n", "other"=>"thing"}}

I can make a pass and run String#strip on all our keys to remove these?

i18n-tasks normalize uses YAML.dump which almost never wraps lines and just uses string quotes

Is there a way for i18n-tasks normalize to ignore wrapping with >? Do we prefer > or string quotes? If string quotes, I would vote to strip them because it looks weird and messy. I like the cleanliness of > and being able to wrap at a comfortable reading line length.

Ok so I have a compromise!

It turns out naive YAML.dump on my machine outputs wraps lines at around 100c by default, without quotes if it can.

I made a script that strips whitespace on YAML files in-place and dumps them back, which has the default line wrapping of 100c.

Then I added a lint spec that makes sure the files are formatted to match this, so hopefully this can help keep our files consistent moving forward?

monfresh · 2017-07-07T23:41:21Z

config/locales/instructions/fr.yml

This needs a space after the colon because there is more text to the right of it that is dynamically displayed by the password strength JS.

monfresh · 2017-07-07T23:42:58Z

config/locales/instructions/en.yml

This space after the colon was probably automatically stripped, but it's required in this case.

Ok, I'll update the scripts to use chomp to remove newlines instead of all whitespace.

PTAL at 0426f5e8e341e8bc84c11e84f8352b8d1462e13e

monfresh · 2017-07-07T23:43:21Z

config/locales/instructions/es.yml

Same here with the missing space after the colon.

monfresh · 2017-07-07T23:55:01Z

spec/i18n_spec.rb

The .new is causing the tests to fail because strip_each is a class method.

monfresh · 2017-07-07T23:55:43Z

spec/lib/yaml_normalizer_spec.rb

s/#strip_each/.strip_each
# is for instance methods, . is for class methods

Yeah this is a result of me flip-flopping on instance vs class, I'll fix it, thanks.

monfresh · 2017-07-07T23:59:44Z

scripts/normalize-yaml

Should we include usage instructions in the README?

Do you think that the note in the i18n_spec.rb spec description for failures that points to the script is close enough? I don't want the README to get too noisy

If this script is meant to be run every time we receive new translations, I think it would be better to give it more visibility, whether in a Wiki or some other place where developers are likely to look up translation-related documentation. What do you think? At the very least, the example in the spec should mention that the command needs a path passed after it. Running scripts/normalize-yaml by itself doesn't do anything. Also, I'm assuming you can give the script a directory, and it will normalize all the files in the directory, but when I tried scripts/normalize-yaml config/locales, it gave me an error. How do you pass in a directory?

This script needs to be run every time we touch translations files (such as when adding new keys to make sure they're alphabetized and wrapped consitently)

To run multiple files, I run:

git ls-files -- config/locales | xargs ./scripts/normalize-yaml

I'll update our translations page in the wiki with this

Thanks! Approved!

monfresh · 2017-07-11T16:13:41Z

config/locales/idv/fr.yml

Looks like this introduced an extra single quote in n''aurez

monfresh · 2017-07-11T16:15:23Z

config/locales/idv/fr.yml

Another extra unnecessary single quote here

monfresh · 2017-07-11T16:17:10Z

Looks like your 4th commit introduced a trailing whitespace in many places that don't need it, as well as unnecessary extra single quotes in words that already have a quote in them.

zachmargolis · 2017-07-11T16:57:20Z

Re: "unnecessary" single quotes, they parse out to the correct thing, are you OK with leaving them in?

YAML.load <<-STR
footnote: 'Vous n''aurez jamais à payer quoi que ce soit avec nous, ni à partager
  le solde de vos comptes. Nous vérifions uniquement le numéro du compte pour
  vérifier votre identité. '
STR
=> {"footnote"=>"Vous n'aurez jamais à payer quoi que ce soit avec nous, ni à partager le solde de vos comptes. Nous vérifions uniquement le numéro du compte pour vérifier votre identité. "}

monfresh · 2017-07-11T17:49:41Z

I personally think it looks weird. Can we revert that change and manually add the extra space in those 3 places that need it?

zachmargolis · 2017-07-11T17:55:51Z

@monfresh which change? There have been so many changes. Do you mean revert the normalization part, or just the #chomp part? Should I add code that special-cases the : suffix and preserves that whitespace?

monfresh

LGTM

**Why**: Better copy **How**: Also run i18n-tasks normalize to make things consistent

**Why**: When we ship translations off to vendors, "2fa" gets turned into the XML tag <2fa> which an invalid tag indentifier, so changing this makes for a friendlier translation pipeline.

**Why**: Makes sure we have consistent formatting, naive YAML.dump adds line wrapping around 100c Also, add special exception for ': ' when trimming whitespace

We've since moved away from vendor which received string content as XML Originally: #1531

* Reimplement password strength as ViewComponent, custom element changelog: Internal, Code Quality, Improve maintainability of password strength UI element * Use score directly in class and strength strings Avoid mappings See: #9826 (comment) * Remove XML-safe check from i18n_spec We've since moved away from vendor which received string content as XML Originally: #1531 * Update specs for PasswordConfirmationComponent * Add specs for PasswordStrengthElement * Arrange conditions in consistent order Easier comparison See: https://github.com/18F/identity-idp/pull/9826/files#r1443220055 Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * Add PasswordStrengthComponent specs * Add PasswordStrengthComponent component preview * Update PasswordToggleComponent to respect custom field ID * Add test coverage for event disavowal password change JavaScript --------- Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>

zachmargolis requested a review from monfresh July 6, 2017 17:30

zachmargolis self-assigned this Jul 6, 2017

zachmargolis added the status - ready for review label Jul 6, 2017

zachmargolis force-pushed the margolis-update-fr-again branch from 25d1f7f to 270cdba Compare July 7, 2017 13:55

monfresh reviewed Jul 7, 2017

View reviewed changes

zachmargolis force-pushed the margolis-update-fr-again branch from a46700c to c546098 Compare July 7, 2017 20:18

monfresh reviewed Jul 7, 2017

View reviewed changes

config/locales/instructions/es.yml Outdated

Copy link

Contributor

monfresh Jul 7, 2017

Choose a reason for hiding this comment

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

Same here with the missing space after the colon.

monfresh reviewed Jul 7, 2017

View reviewed changes

spec/i18n_spec.rb Outdated

Copy link

Contributor

monfresh Jul 7, 2017

Choose a reason for hiding this comment

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

The .new is causing the tests to fail because strip_each is a class method.

monfresh reviewed Jul 7, 2017

View reviewed changes

monfresh reviewed Jul 11, 2017

View reviewed changes

config/locales/idv/fr.yml Outdated

Copy link

Contributor

monfresh Jul 11, 2017

Choose a reason for hiding this comment

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

Looks like this introduced an extra single quote in n''aurez

monfresh reviewed Jul 11, 2017

View reviewed changes

config/locales/idv/fr.yml Outdated

Copy link

Contributor

monfresh Jul 11, 2017

Choose a reason for hiding this comment

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

Another extra unnecessary single quote here

zachmargolis force-pushed the margolis-update-fr-again branch from 92960cf to aae7cd8 Compare July 11, 2017 18:31

monfresh approved these changes Jul 11, 2017

View reviewed changes

zachmargolis force-pushed the margolis-update-fr-again branch from aae7cd8 to a97bddd Compare July 11, 2017 18:54

zachmargolis added 3 commits July 11, 2017 14:54

Update French translations again

8149fb1

**Why**: Better copy **How**: Also run i18n-tasks normalize to make things consistent

Rename 2fa translation key to mfa

6de7985

**Why**: When we ship translations off to vendors, "2fa" gets turned into the XML tag <2fa> which an invalid tag indentifier, so changing this makes for a friendlier translation pipeline.

Implement custom YAML normalization

a97bddd

**Why**: Makes sure we have consistent formatting, naive YAML.dump adds line wrapping around 100c Also, add special exception for ': ' when trimming whitespace

zachmargolis merged commit 2a952e6 into master Jul 11, 2017

zachmargolis deleted the margolis-update-fr-again branch July 11, 2017 19:34

aduth added a commit that referenced this pull request Jan 22, 2024

Remove XML-safe check from i18n_spec

ccc2597

We've since moved away from vendor which received string content as XML Originally: #1531

Conversation

zachmargolis commented Jul 6, 2017

Uh oh!

zachmargolis commented Jul 7, 2017

Uh oh!

zachmargolis commented Jul 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfresh Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfresh commented Jul 11, 2017

Uh oh!

zachmargolis commented Jul 11, 2017

Uh oh!

monfresh commented Jul 11, 2017

Uh oh!

zachmargolis commented Jul 11, 2017

Uh oh!

monfresh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

monfresh Jul 7, 2017 •

edited

Loading