Re: [libgpiod v2][PATCH 0/6] documentation and other minor tweaks

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Fri, Mar 11, 2022 at 8:39 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> The bulk of this series is the documentation tweak pass I promised when
> reviewing other v2 changes.  The rest is a collection of minor things
> I tripped over in the process.
>
> The first is expanding the usage of size_t within the library to cover
> the occasions loop variables should also be size_t.  A noteable exception
> is for offsets, but those are defined as u32 in the uAPI and unsigned int
> in the libgpiod API, so adding another intermediate type seemed like a
> bad idea.
>
> The second and fifth are function renaming for consistency.
>
> The third and fourth are just renaming variables for clarity.
>
> The sixth is the actual documentation tweaks.
>
> The changes and reasoning behind them is as follows:
>
> Fix a few typos.
>
> Add "::" to symbols doxygen links where missing.
>
> Use "\p" to refer to parameters.
>
> Fix space before tabs (triggers a checkpatch warning even if the line
> isn't changed).
> Indentation uses tabs then spaces throughout.
>
> Change "@param offset Offset of the line" to "@param offset The offset of
> the line" to avoid checkpatch repeated word warnings :-/.
>
> Use "Get" to describe getters, rather than "Read".
>
> Use "function" not "routine".
>
> Drop "GPIO" from descriptions where it doesn't add anything.
>
> Drop use of current/currently as it is clear it couldn't be otherwise,
> and adding "current" just makes this reader wonder how to access
> non-current.
>
> Some rewording to improve clarity.
>
> Add some @notes to cover misconceptions or questions I frequently see.
>
> The API is all about chips and lines.
> Recognise that "offset" is an identifier for a line, just as "name"
> could be. So don't use "offset" as a synonymous placeholder for line - use
> line.
>
> Use "num_lines" instead of "num_offsets" or "num_values".
> offsets and values are just attributes of lines, so num_offsets =
> num_values = num_lines, and num_lines is always appropriate, independent
> of which set of attributes are being described.
>
> Use of the definite and indefinite article:
>
> In general, where something is not unique it is described using the
> indefinite article, "a", but if it is unique, including where some
> selection has already been performed, then use the definite article,
> "the".
>
> Only use "this" to emphasise a specific item selected from a set,
> such as when referring to "this function".
> Generally "the" is better, and avoids any possible confusion with C++
> this.
>
> Generally use the indefinite article for @brief descriptions.
> e.g. "The function does something to a thing."
> rather than "The function does something to the thing.", as it is up to
> the caller to make the selection as to which definite thing to call the
> function on.
>
> For containers, an attribute of the contained element is definite, but the
> element itself is indefinite:
> "Clear the edge detection override for a line."
>
> For snapshots, like line_info, the "line" becomes definite as the act of
> taking the snapshot selects the line.
> So "Get the name of the line."
>
> The @param and @return use the definite article as they either identify
> the article, or refer to a specific article, not the generic operation
> of the function like @brief.
>
>
> Do NOT ask me to split those out into separate patches ;-).
>
>
> I realise this aimed at a moving target, so I'm rushing this out a little.
> The commit that this is based off is indicated below - the current
> next/libgpiod-2.0 head at time of writing.
>

Hi Kent!

Thanks for taking the time. I'm mostly ok with those changes,
especially the documentation as I don't really know better (ESL). I
think I may change a couple minor things in the first patches but
nothing big. As far as patch attribution goes - do you want me to
apply it to next/libgpiod-2.0 now and squash it later with your name
mentioned in the commit message or do you want me to queue them for
later once the C++ and Python parts are in as well?

Bart

> Cheers,
> Kent.
>
> Kent Gibson (6):
>   treewide: use size_t for loop variable where limit is size_t
>   API: rename gpiod_request_config_get_num_offsets to
>     gpiod_request_config_get_num_lines to match line_request pattern
>   line-config: rename off to idx
>   line-config: rename num_values to num_lines
>   line-request: rename wait and read functions
>   doc: API documentation tweaks
>
>  include/gpiod.h              | 712 +++++++++++++++++++----------------
>  lib/edge-event.c             |   2 +-
>  lib/line-config.c            |  36 +-
>  lib/line-info.c              |   2 +-
>  lib/line-request.c           |  10 +-
>  lib/request-config.c         |  26 +-
>  tests/tests-edge-event.c     |  38 +-
>  tests/tests-line-request.c   |   2 +-
>  tests/tests-request-config.c |   8 +-
>  tools/gpioget.c              |   4 +-
>  tools/gpioinfo.c             |   4 +-
>  tools/gpiomon.c              |   4 +-
>  tools/gpioset.c              |   6 +-
>  13 files changed, 449 insertions(+), 405 deletions(-)
>
>
> base-commit: 6e15b78d6e9c956c295c755aed793ffd963b1c53
> --
> 2.35.1
>