Re: [PATCH 2/3] Documentation: document naming schema for struct-related functions

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2872 bytes --]

On Wed, Jul 24, 2024 at 09:50:40AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > + - Functions that operate on a specific structure and which are used by
> > +   other subsystems shall be named after the structure.
> 
> I am not sure if this is a good guideline.  In the case of strbuf_,
> you could say it is named after the structure, but I would actually
> think that both structure and the functions are named after the
> subsystem/API (i.e. we have "strbuf" that other subsystems can use).

Well, in most cases I'd expect that the structure is named after the
subsystem/API, itself. I'm happy to relax this statement though and say
that functions should be named after the subsystem.

> > + The function
> > +   name should start with the name of the structure followed by a verb.
> > +   E.g.
> > +
> > +	struct strbuf;
> > +
> > +	void strbuf_add(struct strbuf *buf, ...);
> > +
> > +	void strbuf_reset(struct strbuf *buf);
> > +
> > +    is preferred over:
> > +
> > +	struct strbuf;
> > +
> > +	void add_string(struct strbuf *buf, ...);
> > +
> > +	void reset_strbuf(struct strbuf *buf);
> 
> Do we want to rename start_command(), finish_command(),
> run_command() and pipe_command()? 

I wouldn't quite go that far for now. We may want to slowly adapt some
parts of our interfaces over time. But my main goal is rather to make
the style consistent for _new_ interfaces we add.

> child_process_start() sounds somewhat ungrammatical.

It does, but I would argue that it is no different from `strbuf_reset()`
and other functions where we have the verb as a trailer. And I have to
say that I find it a ton easier to reason about code where we have the
subsystem it belongs to as a prefix as it neatly groups together things
and immediately sets you into the correct mindset of what to expect.
That is of course a question of preference, I'm not claiming that my
preferral is objectively the best.

But again, what I do want to see is consistency. Nobody is helped when
we mix both styles in my opinion. It makes writing, reading and
reviewing code harder than it has to be because you always have to
remember whether it is `string_list_free()`, `free_string_list()`,
`string_list_clear()` or `clear_string_list()`.

> By the way, some functions that have strbuf_ in their names do not
> have anything to do with managing strings using the strbuf
> structure, but they do things that are *not* about strings, but
> happen to use strbuf as a way to either feed input to them or carry
> output out of them.  They should be renamed away to lose "strbuf_"
> in their names (e.g. strbuf_realpath() is about pathnames; it is
> immaterial that the function happens to use strbuf to hold its
> output but takes input from "const char *").

Yeah, that's fair indeed.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]