Re: [PATCH 05/20] git-p4: convert descriptive class and function comments into docstrings

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 12, 2022, 19:31 UTC
Message-ID: <CAPig+cRemRqk44csVJ+QRiZ0MPZpMwzNCSFLCz+qFdh-pcc5KQ@mail.gmail.com>
In-Reply-To: <20220112134635.177877-6-jholdsworth@nvidia.com>

On Wed, Jan 12, 2022 at 8:47 AM Joel Holdsworth <jholdsworth@nvidia.com> wrote:

Show 22 quoted lines

> Previously, a small number of functions, methods and classes were
> documented using comments. This patch improves consistency by converting
> these into docstrings similar to those that already exist in the script.
>
> Signed-off-by: Joel Holdsworth <jholdsworth@nvidia.com>
> ---
> diff --git a/git-p4.py b/git-p4.py
> @@ -1332,13 +1333,15 @@ def getClientRoot():
> -# P4 wildcards are not allowed in filenames.  P4 complains
> -# if you simply add them, but you can force it with "-f", in
> -# which case it translates them into %xx encoding internally.
> -#
>  def wildcard_decode(path):
> -    # Search for and fix just these four characters.  Do % last so
> +    """Decode P4 wildcards into %xx encoding
> +
> +       P4 wildcards are not allowed in filenames.  P4 complains if you simply
> +       add them, but you can force it with "-f", in which case it translates
> +       them into %xx encoding internally.
> +       """
> +
> +    # Search for and fix just these four characters. Do % last so

The unnecessary whitespace change in the "Search for and fix..." line makes for a noisier diff and wasted a little bit of review time. Don't know if it's worth a re-roll, though.

Show 5 quoted lines

> @@ -3006,9 +3020,9 @@ def encodeWithUTF8(self, path):
> -    # output one file from the P4 stream
> -    # - helper for streamP4Files
>      def streamOneP4File(self, file, contents):
> +        """Output one file from the P4 stream - helper for streamP4Files."""

The hyphen is slightly difficult to interpret. A double hyphen, or even better a semicolon, would have helped set off the second phrase from the first. Alternatively, writing it as:

    """Output one file from the P4 stream.

        This is a helper for streamP4Files().
        """

might be even clearer. Probably not worth a re-roll, though.