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

Skip unchanged files in course content sync #36068

Merged
merged 7 commits into from
Aug 18, 2020

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented Jul 29, 2020

Extends our earlier improvements to the sync out to also include course_content files.

Note that unlike the earlier improvements, this one comes with a slight change to functionality. Specifically, in order to be able to skip parsing some files without losing the data in them, we now merge the new data from the sync with the existing data. This means that removing a string from crowdin will no longer remove it from our system. I would definitely consider this a downside, but a relatively minor one compared to the significant time savings this change gets us. I am however quite open to discussion on that point!

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

@Hamms Hamms requested a review from a team as a code owner July 29, 2020 22:53
sanitize_data_and_write(type_data, "dashboard/config/locales/#{type}.#{locale}.#{extension}")
type_file = "dashboard/config/locales/#{type}.#{locale}.#{extension}"

existing_data = YAML.load_file(type_file).dig(locale, "data", type) || {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't these json files 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.

d'oh! That's what I get for testing this before the other changes out there got merged

Copy link
Contributor

@bethanyaconnor bethanyaconnor left a comment

Choose a reason for hiding this comment

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

Approach makes sense to me, just one question on file types.

@Hamms Hamms merged commit c0ddec8 into staging Aug 18, 2020
@Hamms Hamms deleted the skip-unchanged-in-course-content branch August 18, 2020 20:04
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