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

Remove out.sh script #22727

Merged
merged 6 commits into from Jun 5, 2018
Merged

Remove out.sh script #22727

merged 6 commits into from Jun 5, 2018

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented May 29, 2018

This one will probably be easiest to review commit-by-commit.

Overall, it moves most of the functionality of the out.sh script into the out.rb script, rather than calling the former from the latter.

The one major change it makes to the out.sh script is to no longer call the fix-ruby-yml.pl script, which I have confirmed we don't actually need for the "out" sync.

Next steps (in future PRs):

  • Verify that we don't need fix-ruby-yml.pl for sync-pegasus.sh and in.sh, either, and remove it entirely.
  • Figure out if we still need to special-case Armenian or not
  • merge codeorg-messages.sh functionality in here, too, so we can also remove that dependency

@Hamms Hamms requested review from tanyaparker and joshlory and removed request for tanyaparker May 29, 2018 22:24
@Hamms
Copy link
Contributor Author

Hamms commented May 29, 2018

@joshlory
Copy link
Contributor

joshlory commented Jun 1, 2018

Diff with whitespace changes ignored: https://github.com/code-dot-org/code-dot-org/pull/22727/files?w=1

@joshlory
Copy link
Contributor

joshlory commented Jun 1, 2018

Are you looking for feedback on the code moved to sync-codeorg-out.rb, or just a review that the functionality hasn't changed?

Copy link
Contributor

@joshlory joshlory left a comment

Choose a reason for hiding this comment

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

Added some feedback on code I know you're just moving from one place to another, feel free to ignore if you don't think it's worth cleaning up at this time.

@@ -31,6 +34,80 @@ def rename_from_crowdin_name_to_locale
end
end

# Distribute downloaded translations from i18n/locales
# back to blockly, apps, pegasus, and dashboard.
def distribute_translations
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to split up this huge method.

The Dashboard, Apps, Blockly Core, and Pegasus steps have similar structure — can we extract a common helper method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looking over this closer, it feels like the main thing these all share in common is the special-casing for Armenian, which I'm pretty sure I'm gonna be able to remove (in a separate PR).

I vote we leave this as-is for now, and when I figure out whether or not I can stop special-casing Armenian I'll improve this then

end
end

puts "Distribution finished! "
Copy link
Contributor

Choose a reason for hiding this comment

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

Trailing spaces in this print line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just a lazy way to overwrite the previous line

end

### Apps
js_locale = locale.tr('-', '_').downcase
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be great to do a one-time rename so we don't need to think about this special case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean like actually move the files? That would probably be a decently large work item

if file_type == "yml"
new_translation = YAML.load_file(new_translation_path)
# Translation begins
if file_type == "yml"
Copy link
Contributor

Choose a reason for hiding this comment

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

@Hamms
Copy link
Contributor Author

Hamms commented Jun 1, 2018

PTAL!

@Hamms
Copy link
Contributor Author

Hamms commented Jun 4, 2018

Verified that we no longer need to special-case Armenian and removed the relevant code

@Hamms Hamms merged commit 75e37eb into staging Jun 5, 2018
@Hamms Hamms deleted the clean-up-sync-out branch June 5, 2018 22:11
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