Re: [PATCH v2 4/9] doc: use only hyphens as word separators in placeholders
flat view
From: Eli Schwartz <hidden>
Date: 2021-10-31 18:59:18
On 10/28/21 12:21 PM, Jean-Noël Avila via GitGitGadget wrote:
From: =?UTF-8?q?Jean-No=C3=ABl=20Avila?= <redacted> 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.
quoted hunk ↗ jump to hunk
@@ -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".
quoted hunk ↗ jump to hunk
`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". 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. -- Eli Schwartz Arch Linux Bug Wrangler and Trusted User