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

From: Eric Sunshine <sunshine@sunshineco.com>
To: Joel Holdsworth <jholdsworth@nvidia.com>
Cc: Git List <git@vger.kernel.org>, Luke Diamand <luke@diamand.org>,
	Junio C Hamano <gitster@pobox.com>,
	Tzadik Vanderhoof <tzadik.vanderhoof@gmail.com>,
	Dorgon Chang <dorgonman@hotmail.com>,
	Joachim Kuebart <joachim.kuebart@gmail.com>,
	Daniel Levin <dendy.ua@gmail.com>,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Ben Keene <seraphire@gmail.com>,
	Andrew Oakley <andrew@adoakley.name>
Subject: Re: [PATCH 05/20] git-p4: convert descriptive class and function comments into docstrings
Date: Wed, 12 Jan 2022 14:31:40 -0500	[thread overview]
Message-ID: <CAPig+cRemRqk44csVJ+QRiZ0MPZpMwzNCSFLCz+qFdh-pcc5KQ@mail.gmail.com> (raw)
In-Reply-To: <20220112134635.177877-6-jholdsworth@nvidia.com>

On Wed, Jan 12, 2022 at 8:47 AM Joel Holdsworth <jholdsworth@nvidia.com> wrote:
> 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.

> @@ -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.