Add command to run .strings file generation
#36
Conversation
| # Attempts to generate the frozen `.strings` file to gets sent to GlotPress | ||
| # after a new release code freeze. |
There was a problem hiding this comment.
| # Attempts to generate the frozen `.strings` file to gets sent to GlotPress | |
| # after a new release code freeze. | |
| # Attempts to generate the frozen `.strings` file—which usually gets sent to GlotPress | |
| # after a new release code freeze—to catch potential warnings early on. |
There was a problem hiding this comment.
Does genstrings emit warnings? Or errors only? The manpage has no mentions of "error" and only one of "warnings", yet the output we filter for is error:. 🤔
| # Attempts to generate the frozen `.strings` file to gets sent to GlotPress | |
| # after a new release code freeze. | |
| # Attempts to generate the frozen `.strings` file—which usually gets sent to GlotPress | |
| # after a new release code freeze—to catch potential errors early on. |
I'm mostly asking because, if it does also print warnings, it might be useful to catch them too (and treat them as errors if possible).
There was a problem hiding this comment.
That is a very good question… that I don't have the answer to 😅
| # The strings generation happens via Fastlane, so we need to install the | ||
| # Ruby gems. |
There was a problem hiding this comment.
It's me again with my arguments against that 80-columns thing 😄
I'm not against splitting comments into multiple lines if they are really long (like on line 3-4), but this really starts to look odd to me when we start doing that just for line-wrapping… a single word or two.
I don't have a strict and explicit rule about this though, but instinctively I think that my unconscious reasoning is, if a comment would be close to fit on a single line, and wrapping it at 80 cols would lead to the 2nd line be less than, say 20% of the width of the first line, that feels too unbalanced and odd-looking to me?
So for example, I'm fine with your case on lines 13-14, but find this wrapping on line 8-9 or the one for 19-20 to be a bit unnecessary? 🤷
Anyway, it's just a hot take, nothing blocking, sorry for the rant 😅
Co-authored-by: Olivier Halligon <[email protected]>
These changes had already been discussed in wordpress-mobile/WordPress-iOS#19553 and applied in the test PR used for it, but I forgot to cherry pick them back on the original one. Once I copied the script over to this project, I did it from the branch without the improvements.
And argument(s), too. See conversation with @AliSoftware here #36 (comment) You might notice the duplicated `| tee $LOGS` call. I considered DRYing the code by assigning `bundle exec fastlane ...` to a variable, e.g. `CMD`, and then do `$CMD | tee $LOGS`, but decided to keep the duplication because it's minimal and keeps the code, IMHO, clearer.
Co-authored-by: Olivier Halligon <[email protected]>
AliSoftware
left a comment
AliSoftware
left a comment
There was a problem hiding this comment.
Left a small nit about good practices around overuse of cat, but not a blocker.
Disabled auto-merge though just in case you want to address it (or not) before merging.
| cat $LOGS \ | ||
| | sed -ne 's/\[.*\].*genstrings: error: /- /p' \ | ||
| | sed -e $'s/\x1b\[[0-9;]*m//g' \ |
There was a problem hiding this comment.
💄 Nitpick: cat $LOGS | sed -ne $CMD creates two shell processes (one for cat then one for sed) so it's usually a better practice to use sed -ne $CMD $LOGS to use a single shell process (and let sed do the file reading directly, instead of cat doing the file IO and sed doing stdin-piping) instead 🙃
I'm actually kinda surprised that shellcheck didn't automatically suggest you that (is it does warn for similar cases of unnecessary uses of cat) 🤷
| cat $LOGS \ | |
| | sed -ne 's/\[.*\].*genstrings: error: /- /p' \ | |
| | sed -e $'s/\x1b\[[0-9;]*m//g' \ | |
| sed -ne 's/\[.*\].*genstrings: error: /- /p' $LOGS \ | |
| | sed -e $'s/\x1b\[[0-9;]*m//g' \ |
(PS: If we want to go even further, I think you could also collapse the to sed invocations into a single invocation with a unified sed command/script doing the replacements all at once… but I'm not convinced that will lead to something super readable, so I'm ok to keep those separate)
Extracts the script developed in wordpress-mobile/WordPress-iOS#19553 to this plugin so it can be used across multiple repos and iterated upon in isolation.
See wordpress-mobile/WordPress-iOS#19624 for a live demo of the happy behavior and wordpress-mobile/WordPress-iOS#19544 for when it catches errors.