Re: [PATCH 1/2] python file more pytonic, adjust "if" and "for"

From: Victoria Dye <vdye@github.com>
Date: Oct 6, 2022, 16:02 UTC
Message-ID: <66d1bcaf-64c8-13c2-ba7a-98715de3617b@github.com>
In-Reply-To: <71da6f53a44cd3390d122ff2c0446824313e5101.1665056747.git.gitgitgadget@gmail.com>

dsal3389 via GitGitGadget wrote:

> From: dsal3389 <dsal3389@gmail.com>

re: the commit message title

s/pytonic/pythonic

Please prefix the title with the file/function being updated. Based on precedent [1], that'd probably be:

    git-p4: <your message>

Also, I'm not sure the reference to "for" is helpful, since you're only adjusting if-statements in this patch (although one of those conditionals does include a for-loop, nothing about the loop itself changes).

Finally, please use the imperative mood in your commit messages [2]; something like:

    git-p4: adjust "if" statements to be more pythonic

[1] https://lore.kernel.org/git/?q=dfn%3Agit-p4.py [2] https://git-scm.com/docs/SubmittingPatches#describe-changes

> 
> L371
> redesign few lines to get rid of the "else" statement

What was your reason for making this change? I don't see anything in PEP8 [3] regarding this kind of conditional organization being preferred over what's already there.

For what it's worth, I'm happy with the change itself; I'd just ask that you include an explanation for why (because it's more concise, or makes the assignment to 'out' clearer, etc.). The "Describe your changes well" section of the "SubmittingPatches" document [2] should be a helpful guideline.

Also, as a matter of convention, it's not necessary to include line numbers.

[2] https://git-scm.com/docs/SubmittingPatches#describe-changes [3] https://peps.python.org/pep-0008/

Show 5 quoted lines

> 
> L404
> moved the if statement below another if statement that
> checks if it should exit the code, only if it doesnt need to,
> then we can iterate the for loop and decode the text

While the actual change below makes sense, I found this explanation very difficult to parse. Maybe something like:

    Reorder if-statements in 'read_pipe_lines()' to raise an error with the 
    command before trying to decode its results in order to avoid 
    unnecessary computation.

> 
> Changes to be committed:
> 	modified:   git-p4.py

Please remove this text from the commit message.

Show 5 quoted lines

> 
> Signed-off-by: Daniel Sonbolian <dsal3389@gmail.com>
> ---
>  git-p4.py | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)

The rest of the implementation looks good, thanks!

Show 31 quoted lines

> 
> diff --git a/git-p4.py b/git-p4.py
> index d26a980e5ac..0ba5115fa2e 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -368,10 +368,9 @@ def read_pipe(c, ignore_error=False, raw=False, *k, **kw):
>         """
>      retcode, out, err = read_pipe_full(c, *k, **kw)
>      if retcode != 0:
> -        if ignore_error:
> -            out = ""
> -        else:
> +        if not ignore_error:
>              die('Command failed: {}\nError: {}'.format(' '.join(c), err))
> +        out = ""
>      if not raw:
>          out = decode_text_stream(out)
>      return out
> @@ -400,10 +399,10 @@ def read_pipe_lines(c, raw=False, *k, **kw):
>      p = subprocess.Popen(c, stdout=subprocess.PIPE, *k, **kw)
>      pipe = p.stdout
>      lines = pipe.readlines()
> -    if not raw:
> -        lines = [decode_text_stream(line) for line in lines]
>      if pipe.close() or p.wait():
>          die('Command failed: {}'.format(' '.join(c)))
> +    if not raw:
> +        lines = [decode_text_stream(line) for line in lines]
>      return lines
>  
>