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

Comprehensive reorganization of localization file #6521

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

andrewboerema
Copy link

Summary
I was doing some work on a feature and a saw how unorganized the localization file was. Not only does it make it a crap shoot for where to put a new string (if you even feel like doing it), it also makes it really difficult to approach translating into a new language at all. To that end, I made my best effort to organize the strings. I have grouped them by command (using common and core for things that don't relate to a specific command) alphabetized the groups, and alphabetized the strings within the groups. I also added a header to explain the organization within the file.

This will make the file much cleaner and more maintainable. It also makes it easy to catch people not adding strings in the right place during code review.

I do understand this will be a pain for a little bit because it will cause a number of merge conflicts. I feel that the organization and maintenance gains will make it worth it, though.

Test plan
If the tests pass, this should be good. I am not intentionally changing any functionality.

@buildsize
Copy link

buildsize bot commented Oct 9, 2018

File name Previous Size New Size Change
yarn-[version].noarch.rpm 1.09 MB 1.09 MB 537 bytes (0%)
yarn-[version].js 4.26 MB 4.27 MB 3.44 KB (0%)
yarn-legacy-[version].js 4.44 MB 4.44 MB 3.44 KB (0%)
yarn-v[version].tar.gz 1.1 MB 1.1 MB -666 bytes (0%)
yarn_[version]all.deb 803.62 KB 803.85 KB 232 bytes (0%)

@arcanis
Copy link
Member

arcanis commented Oct 13, 2018

The review will be fun, but this is a really good idea thanks for tackling this! I love "better engineering" PRs like this one 🙂

Copy link
Member

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

What would you think of adding a character to split namespace and key? IepackageResolver/incorrectLockfileEntry instead of packageResolverIncorrectLockfileEntry. I think that would help readability

@@ -210,7 +210,7 @@ export default class Git implements GitRefResolvingInterface {
if (await Git.repoExists(secureRef)) {
return secureRef;
} else {
reporter.warn(reporter.lang('downloadHTTPWithoutCommit', ref.repository));
reporter.warn(reporter.lang('getDownloadHttpWithoutCommit', ref.repository));
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be gitDownloadHttpWithoutCommit (rather than get)?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I will update.

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.

2 participants