-
Notifications
You must be signed in to change notification settings - Fork 5
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
Ensure localization catalogs don't go out of date. #1443
Conversation
chalk.redBright( | ||
`Extracted message catalogs are out of date! ` + | ||
`Please re-run "${extractCmd}", or commit the changes to .po ` + | ||
`files that have just been made on this machine.` | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! At first glance of this, it wasn't fully clear to me that this command yarn lingui:check
was also fully running the extract
command as well and updating our messages.po files, perhaps because my assumption is that any "check" command is essentially "returning a boolean" in that it doesn't change anything, just determines whether something is as we want it to be. That being said, it makes sense that of course you'd need to run the extract
command in order to do the check. Also, the fact that the results get saved is very helpful as my instinct from seeing a failed check would be to run extract
again and commit the updates.
So, all in all, my only potential suggestion would be to either rename the command to be more explicit about also running the extract command in full (something like "extract-and-check"? Although that sounds a bit wordy), and/or changing the wording of the warning message on failed checks to be clearer that a full extract took place (maybe something like "We extracted the message catalogs and they are out of date! Please commit these changes or ...")?
Hmm, now I'm feeling like this is super knit-picky and maybe a non-monday-morning brain would've interpreted things better. Feel free to ignore these suggestions if the current implementation fits other similar features!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I think your criticisms totally make sense! It was a bit frustrating to implement actually because there's no way (as far as I can tell) to figure out whether everything is copacetic without actually running extract--my original intent was to make "check" just return a boolean but not make any modifications to the filesystem, so your initial assumption matches my intent, but then I found out there's not actually a way to do it without having "side effects" so to speak.
I like the idea of yarn:extract-and-check
though! It is a bit verbose but that's OK because it's only really run in CircleCI, so I'll go ahead and make that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome, @toolness! I have one knit-picky comment about naming/messaging but that's it.
This fixes #1442 by checking to make sure the content of localization catalogs never goes out-of-date. Note that it does not make sure that file/line number references are accurate, since that's likely to change frequently and doesn't affect the accuracy of our translations (although it might be confusing for translators, I don't think we should consider such inaccuracies a broken build).
Unfortunately, though, this does have a CI impact; on my system the new
yarn lingui:extract-and-check
command takes about 18 seconds to run, primarily because it needs to runyarn lingui:extract
.