[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] CODING_STYLE: Add a section of the naming convention



Hi George,

On 06/12/2023 16:22, George Dunlap wrote:
On Wed, Dec 6, 2023 at 11:22 AM Julien Grall <julien@xxxxxxx> wrote:

Hi,

On 06/12/2023 11:19, Andrew Cooper wrote:
On 06/12/2023 8:41 am, Jan Beulich wrote:
On 06.12.2023 03:21, George Dunlap wrote:
On Tue, Dec 5, 2023 at 6:12 PM Julien Grall <julien@xxxxxxx> wrote:
From: Julien Grall <jgrall@xxxxxxxxxx>

Several maintainers have expressed a stronger preference
to use '-' when in filename and option that contains multiple
words.

So document it in CODING_STYLE.

Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

---
      Changes in v2:
          - New wording
          - Update the section title
          - Add Jan's acked-by
---
   CODING_STYLE | 9 +++++++++
   1 file changed, 9 insertions(+)

diff --git a/CODING_STYLE b/CODING_STYLE
index ced3ade5a6fb..ed13ee2b664b 100644
--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -144,6 +144,15 @@ separate lines and each line should begin with a leading 
'*'.
    * Note beginning and end markers on separate lines and leading '*'.
    */

+Naming convention for files and command line options
+----------------------------------------------------
+
+'-' should be used to separate words in commandline options and filenames.
+E.g. timer-works.
+
+Note that some of the options and filenames are using '_'. This is now
+deprecated.
Sorry for not catching this last time; "are using X" isn't really
idiomatic English; more idiomatic would be something like the
following:

"Note that some existing options and file names use '_'.  This is now
deprecated."

Since we're changing things, I *think* most style guides would advise
against starting the sentence with a punctuation; so perhaps:

"Command-line options and file names should use '-' to separate words;
e.g., timer-works."

And what about adding to the last paragraph:

"When touching code around command-line parameters still using '_', it
is recommended to modify the documentation to say only '-', but modify
the code to accept both '-' and '_' (for backwards compatibility)."
In this context see
https://lists.xen.org/archives/html/xen-devel/2020-01/msg01945.html
and Andrew's response
https://lists.xen.org/archives/html/xen-devel/2020-01/msg02006.html
I'm still in favor of addressing the issue centrally (making unnecessary
adjustments like you suggest in the new paragraph). Yet I think Andrew's
objection would cover such adjustments as much as my generic solution.

Aliasing - and _ in the cmdline parsing breaks basic usability.

Its fine for new options to use -, and it's even fine-ish (but only if
you're going to be the one doing security backports) to rename internal
files.

But there is real and detrimental effect for altering the command line.

You will get people failing to express the option they intended when
working with an older form of Xen.  You will need an absurd number of
notes in the command line docs saying "newer versions of Xen accept an
alias but you need to use the underscore form for backwards compatibility".

Not to mention that there are years of notes scattered all around the
internet using the underscore forms, so it's likely that everyone will
continue to use the underscore form, meaning that you don't even have a
way to phase them out.

And for what?  An attempt to pretend that we don't have 2 decades of
history where underscores where the norm?

It's tinkering, for no useful benefit and a clear cost.

+1 with what Andrew said.

Haven't given it full thought, because I absolutely did not want to
make this change take longer to get in. :-). The existence of
disagreement is enough for me to withdraw my suggestion.

Thanks. I will commit the patch soon then. I am happy to continue the discussion though. I would love to get the file CODING_STYLE reflecting more our style policies :).

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.