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

Commit Message

Daniel Sonbolian Oct. 6, 2022, 11:45 a.m. UTC

From: dsal3389 <[e-mail]>

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

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

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

Signed-off-by: Daniel Sonbolian <[e-mail]>
---
 git-p4.py | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Victoria Dye Oct. 6, 2022, 4:02 p.m. UTC | #1

dsal3389 via GitGitGadget wrote:
> From: dsal3389 <[e-mail]>

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/

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

> 
> Signed-off-by: Daniel Sonbolian <[e-mail]>
> ---
>  git-p4.py | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)

The rest of the implementation looks good, thanks!

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

Junio C Hamano Oct. 6, 2022, 5:16 p.m. UTC | #2

"dsal3389 via GitGitGadget" <[e-mail]> writes:

> From: dsal3389 <[e-mail]>
>
> L371
> redesign few lines to get rid of the "else" statement
>
> 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
>
> Changes to be committed:
> 	modified:   git-p4.py

Compare this with the commits by others in "git log --no-merges"
output of this project.

> Signed-off-by: Daniel Sonbolian <[e-mail]>

Please have this on the in-body "From:" line we see above.  I think
GitGitGadget takes it from the author of the commit object it sends
out, so you may have to go back to your branch and fix them with
"rebase -i".

> ---
>  git-p4.py | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> 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 = ""

I think the code with or without the patch is about the same
complexity, but people tend to have harder time understanding logic
that involves double negation, so I can imagine that some readers
may find the code with the patch harder to understand.  In any case,
the difference falls into the "it is minor enough that once it is
written in one way, it is not worth the churn to rewrite it in the
other way" category.

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

This is in the same "the difference is minor enough that once it is
written in one way, it is not worth the churn to rewrite it in the
other way" category.  Your reasoning might be that massaging of the
lines is only needed when we do not die() and it is more efficient
to check and die first, but that is optimizing for the wrong case.
The code should not die in its normal operation and there is no
point optimizing for an error code path.

One thing that might deserve benchmarking and optimizing here is if
we can do better than reading everything in lines array and holding
onto the original until decoding the whole thing at once at the end
of input.  If converting each line and appending the result as it is
read from the pipe turns out to be more efficient, it may be an
optimization worth considering, as it is optimizaing for the normal
case.

Patch

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