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

Fix i18n paths for hourofcode project #33002

Merged
merged 3 commits into from Feb 12, 2020
Merged

Fix i18n paths for hourofcode project #33002

merged 3 commits into from Feb 12, 2020

Conversation

bethanyaconnor
Copy link
Contributor

@bethanyaconnor bethanyaconnor commented Feb 5, 2020

This PR moves the files downloaded from Crowdin into i18n/locales/<locale>/hourofcode and distributes them from there.

Testing

Dry run with new config:

Version 2.0.31 of the CLI client is available. You can download the new version here: https://downloads.crowdin.com/cli/v2/crowdin-cli.zip
activity-guidelines.md
advisory-committee.md
assistive-technology.md
en.yml
how-to/2019.md
how-to/afterschool.md
how-to/companies.md
how-to/districts.md
how-to/events.md
how-to/index.md
how-to/parents.md
how-to/public-officials.md
how-to/volunteers.md
international-partners.md
partners.md
promote/index.md
promote/official-press-release.md
promote/op-ed.md
promote/press-kit.md
promote/previous-posters.md
promote/proclamation.md
promote/resources.md
promote/stats.md
supporting-special-needs-students.md
thanks.md
whole-school.md

Which looks the same as the current set of files in the hour-of-code project.

I ran the sync out and after pulling all the files from the hourofcode project.

Links

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@bethanyaconnor bethanyaconnor marked this pull request as ready for review February 6, 2020 17:23
@bethanyaconnor bethanyaconnor requested a review from a team as a code owner February 6, 2020 17:23
"source": "/**/*.md",
"translation": "/%language%/hourofcode/**/%file_name%.md",
"source": "/source/hourofcode/**/*.md",
"translation": "%language%/hourofcode/**/%file_name%.md",
Copy link
Contributor

Choose a reason for hiding this comment

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

is this intentional? Specifically, is it intentional that the translation field for en.yml has a leading slash and this one doesn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh that looks like a mistake. I'll fix that


# API Credentials must be loaded from a separate identity file. See
# https://support.crowdin.com/configuration-file/#split-project-configuration-and-api-credentials
"api_key" : ""

"preserve_hierarchy": true
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we want to preserve this; otherwise, source files will all be uploaded to the root directory of the crowdin project

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They were being uploaded into the source directory of the crowdin project with it, which was extra strange

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The dryrun above was without this flag and it looks right? The code-dot-org crowdin project doesn't have this setting so I'm not sure why this one is different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I read the documentation and it isn't clear. If preserve_hierarchy is set to true it will preserve the whole hierarchy but I'm not sure what happens when it's not set.

Copy link
Contributor

Choose a reason for hiding this comment

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

upon further investigation, it looks like we actually aren't using it in the config for the regular codeorg sync either, so I think I had some misunderstanding about how this is used

next unless File.directory?(crowdin_dir)

FileUtils.cp_r File.join(crowdin_dir, '.'), dest_dir
FileUtils.rm_r crowdin_dir
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we still need this operation, and also the "remove unused" one below? Before, translations would get downloaded to (for example) /i18n/locales/source/hourofcode/Spanish/ and now they'll be downloaded to /i18n/locales/Spanish/hourofcode/; we want them to be in /i18n/locales/es-MX/hourofcode/; the work needed hasn't changed, just the specific location of the source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the sync-out does that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specifically this function now also copies the hourofcode downloaded files:

def rename_from_crowdin_name_to_locale

Copy link
Contributor

Choose a reason for hiding this comment

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

ooooooh of course. I forgot to consider that these syncs are unified now

@bethanyaconnor bethanyaconnor merged commit 7b608a8 into staging Feb 12, 2020
@bethanyaconnor bethanyaconnor deleted the fix-i18n-paths branch February 12, 2020 17:56
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.

None yet

2 participants