Reuse server strings when possible in stats #3442
Conversation
|
We actually have a test ensuring we don't do this, so I'm assuming it is intentional? see https://app.circleci.com/pipelines/github/metabrainz/musicbrainz-server/12638/workflows/b2485960-7220-46b2-9416-38fecdf44aa9/jobs/20969 @yvanzo, can you remind us why? |
|
See #3139 and #3141, particularly #3141 (comment). I think the gist is:
I'm not qualified to speak to the second point, though. |
|
Ah, ok. Well, I do feel that there's some cases where this makes sense, but I guess if we wanted to do it it could be made more explicit (something like aliasing it to |
|
Adding an explicit |
reosarevok
force-pushed
the
less-split-statistics
branch
from
a3dd3cc to
71f4c0b
Compare
We were making people translate stuff twice a lot more than strictly needed, and mwiencek suggested we don't. I'm sure I've missed some reused strings, but this is most of them. To ensure we don't accidentally use l for statistics strings, this explicitly imports l as lMbServer and uses that. We have a test to prevent bare l(), but it doesn't seem to work at the moment since it wasn't triggered by this.
reosarevok
force-pushed
the
less-split-statistics
branch
from
71f4c0b to
0e9808a
Compare
1. The `!` negation does not actually cause the script to exit (despite `set -e`)
if the `git grep` succeeds (finds matches). See [1]:
"The shell does not exit if the command that fails is part of the command
list immediately following a while or until keyword, part of the test in
an if statement, part of any command executed in a && or || list except
the command following the final && or ||, any command in a pipeline but
the last, or if the command’s return status is being inverted with !."
Thus I've replaced the `!` at the beginning of the command with
`&& { exit 1; }` at the end.
2. It appears that the tests could hang on the `git grep` command if it
produced too many matches due to the results being paged. I've also added
the `--no-pager` flag to resolve this.
[1] https://www.gnu.org/software/bash/manual/html_node/The-Set-Builtin.html:
|
Did you test the test works when you break some stuff? :) I pushed an extra commit to make eslint allow |
This was blocked by eslint but we have exceptions for all others, and looks a lot more readable than lMbServer (and related names)
reosarevok
force-pushed
the
less-split-statistics
branch
from
de73f2d to
c58644d
Compare
These aren't used yet, but it seems likely they will be in the future, so it shouldn't hurt to add them.
I had tested it manually by opening a |
Description
We were making people translate stuff twice a lot more than strictly needed, and @mwiencek suggested we don't. I'm sure I've missed some reused strings, but this is most of them.
On top of #3392 because that's where this started and this has strings from there.