Re: [PATCH 17/20] git-p4: compare to singletons with "is" and "is not"

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 12, 2022, 19:54 UTC
Message-ID: <CAPig+cSHwN9N1K1Fi2DbZhgRdi=iqBkYB==rMKsrq6vqPQJQ4Q@mail.gmail.com>
In-Reply-To: <20220112134635.177877-18-jholdsworth@nvidia.com>

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

Show 16 quoted lines

> PEP8 recommends that comparisons with singletons such as None should be
> done with "is" and "is not", and never equality operators.
>
> This guideline is described here:
> https://www.python.org/dev/peps/pep-0008/#programming-recommendations
>
> Signed-off-by: Joel Holdsworth <jholdsworth@nvidia.com>
> ---
> diff --git a/git-p4.py b/git-p4.py
> @@ -930,7 +930,7 @@ def p4Where(depotPath):
>              if data[:space] == depotPath:
>                  output = entry
>                  break
> -    if output == None:
> +    if not output:
>          return ""

This changes semantics, doesn't it? It will now return "" for None, False, and "". Is this change intentional? To match the intent of the original, I would have expected it to be written:

    if output is None:
        return ""

but, having glanced at the larger context, I suppose the new semantics may be okay(?).

Show 6 quoted lines

>  def parseRevision(ref):
> @@ -4497,7 +4497,7 @@ def main():
>      if cmd.needsGit:
> -        if cmd.gitdir == None:
> +        if not cmd.gitdir:
>              cmd.gitdir = os.path.abspath(".git")

Same question/observation as above. In order to save reviewers some time, perhaps the commit message could say something about why the semantic change is safe and/or desirable.