Re: [PATCH v2] CodingGuidelines: discourage arbitrary suffixes in function names

From: Justin Tobler <jltobler@gmail.com>
Date: Oct 23, 2024, 23:07 UTC
Message-ID: <vshoz6xw3d3kghi7lsag6ldike6pdphkjbst5afxbuvjeyprxd@e4ectkxbh4v7>
In-Reply-To: <Zxld62svV9iDCcJJ@nand.local>

On 24/10/23 04:34PM, Taylor Blau wrote:

Show 20 quoted lines

> On Wed, Oct 23, 2024 at 09:57:06AM +0200, Karthik Nayak wrote:
> > + - Function names should be self-explanatory, clearly reflecting their
> > +   purpose or behavior.
> > +
> > +   The '_1' suffix for function names has historically indicated:
> > +
> > +    - functions processing one of several elements that all need to be
> > +      handled similarly.
> > +
> > +    - recursive functions that need to be separated from a setup stage.
> > +
> > +   To maintain clarity and avoid confusion, such arbitrary suffixes are
> > +   discouraged, as they provide no meaningful insight into the function's
> > +   role.
> > +
> 
> I'm still not sold on the suggestion to discourage the use of '_1' in
> the future, so we may want to further qualify this statement with cases
> where it is OK (in the spirit of Patrick's "as long as this is loosely
> applied" comment from earlier).

When I fist encountered the use of the '_1' suffix, I was also not immediately sure of the intent it was trying to communicate. If I remember correctly, there are also existing uses that don't really fit into the two examples mentioned above. One such use I found is `handle_revision_arg_1()`[1] where it seems we are just more generally factoring out a subset of the parent functions logic, but keeping the function name mostly the same. If this is also a usecase, maybe we should instead say something more generic along the lines of:

"A function suffixed with '_1' indicates that a subset of logic from its corresponding parent function has been factored out and encapsulated in a separate function."

Regarding whether its use should be discouraged, if the convention is well understood and documented it probably isn't much of an issue, but for newcomers it may be another source of confusion as its meaning is not implicitly well understood. I do think its a good first step to improve the documentation here though. :)

-Justin

[1] https://gitlab.com/gitlab-org/git/-/blob/fd3785337beb285ed7fd67ce6fc3d3bed2097b40/revision.c#L2176