On Fri, Mar 12, 2021 at 09:33:46AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>
> On Thu, Mar 11 2021, Emily Shaffer wrote:
>
> > + switch (should_run_hookdir) {
> > + case HOOKDIR_NO:
>
> Style: case shouldn't be indented
Done, thanks.
>
> > + strbuf_addstr(&hookdir_annotation, _(" (will not run)"));
> > + break;
> > + case HOOKDIR_ERROR:
> > + strbuf_addstr(&hookdir_annotation, _(" (will error and not run)"));
> > + break;
> > + case HOOKDIR_INTERACTIVE:
> > + strbuf_addstr(&hookdir_annotation, _(" (will prompt)"));
> > + break;
> > + case HOOKDIR_WARN:
> > + strbuf_addstr(&hookdir_annotation, _(" (will warn but run)"));
> > + break;
> > + case HOOKDIR_YES:
> > + /*
> > + * The default behavior should agree with
> > + * hook.c:configured_hookdir_opt(). HOOKDIR_UNKNOWN should just
> > + * do the default behavior.
> > + */
> > + case HOOKDIR_UNKNOWN:
> > + default:
> > + break;
>
> We should avoid this sort of translation lego.
>
> > + }
> > +
> > list_for_each(pos, head) {
> > struct hook *item = list_entry(pos, struct hook, list);
> > item = list_entry(pos, struct hook, list);
> > if (item) {
> > /* Don't translate 'hookdir' - it matches the config */
> > - printf("%s: %s\n",
> > + printf("%s: %s%s\n",
>
> native speakers in some languages to read the sentance backwards.
> Because if you concatenate strings like this you force.
>
> (We don't currently have a RTL language in po/, still, but let's not
> create churn for if/when we do if we can help it)>
Yeah, you are absolutely right. I'll take a look at your suggestion,
thanks.
>
>
> I have a patch on top to fix this, will send it as some general reply of
> proposed fixup.s
FWIW, I found this format of suggestions really hard to navigate. I had
to go find your fixup (and I think you sent two different ones) and then
had to scroll around and find what you're referring to from here. If I
were to apply the fixup directly, I'd have to split it up and find the
appropriate commits to associate each part of the diff with. I know
generating scissors patches for each review is a pain, but I'd even
prefer a prose "How about printing in each part of the case instead, so
each string can be translated in full" or a non-formatted example inline
to the catchall fixups patch you sent.
>
> > (item->from_hookdir
> > + git hook list pre-commit >actual &&
> > + # the hookdir annotation is translated
> > + test_i18ncmp expected actual
>
> This (and the rest of test_i18ncmp in this series) can and should just
> be "test_cmp" or "test_i18ncmp", the poison mode is dead. See my recent
> patches to search/replace test_i18ncmp.
>
> The reason the function isn't gone entirely was to help a series like
> yours in "seen", but if we're re-rolling...
Oh cool, thanks, will do.
