Le dimanche 31 octobre 2021, 19:58:56 CET Eli Schwartz a écrit : > On 10/28/21 12:21 PM, Jean-Noël Avila via GitGitGadget wrote: > > From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <jn.avila@xxxxxxx> > > > > According to CodingGuidelines, spaces and underscores are not > > allowed in placeholders. > > > I have a patch under review that touches the same files you are > modifying here. As I've been pointed to these changes, I'd like to make > a quick observation. > > > > @@ -101,9 +101,9 @@ commits are displayed, but not the way the diff is shown e.g. with > > `git log --raw`. To get full object names in a raw diff format, > > use `--no-abbrev`. > > > > -* 'format:<string>' > > +* 'format:<format-string>' > > + > > -The 'format:<string>' format allows you to specify which information > > +The 'format:<format-string>' format allows you to specify which information > > you want to show. It works a little bit like printf format, > > with the notable exception that you get a newline with '%n' > > instead of '\n'. > > @@ -273,12 +273,12 @@ endif::git-rev-list[] > > If any option is provided multiple times the > > last occurrence wins. > > + > > -The boolean options accept an optional value `[=<BOOL>]`. The values > > +The boolean options accept an optional value `[=<value>]`. The values > > > Here you change "BOOL" to "value", below you change it to "bool-value". Indeed. Should be fixed. > > > > `true`, `false`, `on`, `off` etc. are all accepted. See the "boolean" > > sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean > > option is given with no value, it's enabled. > > + > > -** 'key=<K>': only show trailers with specified key. Matching is done > > +** 'key=<key>': only show trailers with specified <key>. Matching is done > > case-insensitively and trailing colon is optional. If option is > > given multiple times trailer lines matching any of the keys are > > shown. This option automatically enables the `only` option so that > > @@ -286,9 +286,9 @@ option is given with no value, it's enabled. > > desired it can be disabled with `only=false`. E.g., > > `%(trailers:key=Reviewed-by)` shows trailer lines with key > > `Reviewed-by`. > > -** 'only[=<BOOL>]': select whether non-trailer lines from the trailer > > +** 'only[=<bool-value>]': select whether non-trailer lines from the trailer > > block should be included. > > -** 'separator=<SEP>': specify a separator inserted between trailer > > +** 'separator=<sep>': specify a separator inserted between trailer > > lines. When this option is not given each trailer line is > > terminated with a line feed character. The string SEP may contain > > the literal formatting codes described above. To use comma as > > @@ -296,15 +296,15 @@ option is given with no value, it's enabled. > > next option. E.g., `%(trailers:key=Ticket,separator=%x2C )` > > shows all trailer lines whose key is "Ticket" separated by a comma > > and a space. > > -** 'unfold[=<BOOL>]': make it behave as if interpret-trailer's `--unfold` > > +** 'unfold[=<bool-value>]': make it behave as if interpret-trailer's `-- unfold` > > option was given. E.g., > > `%(trailers:only,unfold=true)` unfolds and shows all trailer lines. > > -** 'keyonly[=<BOOL>]': only show the key part of the trailer. > > -** 'valueonly[=<BOOL>]': only show the value part of the trailer. > > -** 'key_value_separator=<SEP>': specify a separator inserted between > > +** 'keyonly[=<bool-value>]': only show the key part of the trailer. > > +** 'valueonly[=<bool-value>]': only show the value part of the trailer. > > +** 'key_value_separator=<sep>': specify a separator inserted between > > trailer lines. When this option is not given each trailer key-value > > pair is separated by ": ". Otherwise it shares the same semantics > > - as 'separator=<SEP>' above. > > + as 'separator=<sep>' above. > > > > NOTE: Some placeholders may depend on other options given to the > > revision traversal engine. For example, the `%g*` reflog options will > > > These changes over here are contrary to the statement in the commit > message. In addition to switching to hyphens, they: > > - switch casing (okay, makes sense, you point this out in the cover > letter but maybe it's worth mentioning it in the commit message too? > idk) > > - change the terms used -- and this I don't understand. I'm not really > bothered by switching <n> to <number> or <k> to <key>, as these > changes seem reasonable (though again, they are not mentioned in the > commit message). However, "bool-value" seems odd. Why that and not > "number-value"? IMHO the "value" is redundant here, let it be "bool" > and "number". My initial aim was to be more descriptive. The placeholders act as variables and you don't name variables with their types. Fair enough, "bool-value" isn't the best example, but there are some facts that drove these choices: * You can't expect manpage readers to be seasoned C programmers that understand what the word "bool" means. Using boolean-value, makes the reference to the description in the previous paragraph. * I'm facing a similar issue with translators. Some of them have some culture of computer science, but most of them are not programmers. Having more meaningful placeholders helps them find a correct translation: this is a bool value, not the bool type. I did not push far to change all the placeholders that were not descriptive. Maybe another set of patches, if it is acceptable. The choices here may be awkward; no problem to propose even more descriptive names. > > Similarly "the 'format:<format-string>' format" feels highly > redundant, I expect the reader knows that <string> contains a format > inside it as it's mentioned immediately before *and* after. > The fact that it is a string doesn't tell you much about what you can do with it. For me, this isn't a problem that the explanation is redundant.
