Travis CI: Lint for Python syntax errors and undefined names

[cclauss]

Several things were changed between Python 2 and Python 3.
Here we are making changes to make the code run on both Py2 and Py3.
We are doing this because the end of life of Python 2 is in 167 days.
We are using print() function because legacy print statements are syntax
errors on Py3.
reduce() was moved in Python 3 and raw_input() was removed so we
make changes to avoid NameErrors at runtime.
Signed-off-by: Christian Clauss [email protected]

[gitgitgadget]

Welcome to GitGitGadget

Hi @cclauss, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that this Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

• the lines should not exceed 76 columns,
• the first line should be like a header and typically start with a prefix like "tests:" or "commit:", and
• the commit messages' body should be describing the "why?" of the change.
• Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a PR comment of the form /allow <username>.

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions.

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox ("raw") file corresponding to the mail you want to reply to from the Git mailing list. If you use GMail, you can upload that raw mbox file via:

curl -g --user "<EMailAddress>:<Password>" --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

[dscho]

/allow cclauss

[gitgitgadget]

User cclauss is now allowed to use GitGitGadget.

[cclauss]

/submit

[gitgitgadget]

Error: Could not determine full name of cclauss

[cclauss]

/submit

[gitgitgadget]

Submitted as [email protected]

[dscho]

@cclauss what GitGitGadget wants is that your GitHub profile mentions your clear name... It needs that so that it can construct a sender name for the cover letter.

[cclauss]

Yes. I edited that parameter on GitHub settings and resubmitted. That seems to have done the trick. Thanks for your help. 🤞

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"cclauss via GitGitGadget" <[email protected]> writes:

> From: cclauss <[email protected]>

I'll tweak this line (and your sign-off) to read as "Christian
Clauss <[email protected]>" as you had in your cover letter.

> Several things were changed between Python 2 and Python 3.
> There are a few Python 3 incompatibilities to work on.
> Here we are making changes to make the code run on both Py2 and Py3.
> We are doing this because the end of life of Python 2 is in 167 days.

All sounds sensible, and the above is quite a good problem
description.

> We are using print() function because legacy print statements are syntax
> errors on Py3.
> reduce() was moved in Python 3 and raw_input() was removed so we make
> changes to avoid NameErrors being raised at runtime.
> We are also putting flake8 lint tests in place on Travis CI to avoid
> any backsliding on future pull requests.

Nothing is wrong here, but the convention in our codebase is to
describe the changes as if we are giving orders to the codebase "to
be like so".  And as you have enumeration, I would write something
like this if I were describing this change:


     - Use the `print()` function, because Py3 no longer has the `print`
       statement.

     - Import `reduce()` from functools, because Py3 requires this, and
       importing also works with Py2 (even though it wasn't necessary).

     - Use `input()` instead of `raw_input()`, as the former can be used
       with both but the latter was removed in Py3.

    Also add a CI job to Travis CI to run flake8 lint to avoid an
    backsliding on future pull requests.

https://python-future.org/compatible_idioms.html#raw-input seems to
suggest, just like you import reduce from functools, you need to
import input from builtins.  Is it not the case?

> +      script: flake8 . --count --select=E9,F63,F72,F82 --show-source --statistics

How was the set of "select"ed violations to catch chosen?  How are
we going to maintain this list going forward?

The rest of the patch looked sensible.

Thanks.

[cclauss]

@gitster Did this get to the other side?

$ git send-email --in-reply-to=[email protected] --to=[email protected] --cc=[email protected] --cc=[email protected] --cc=[email protected] ./note_to_git.txt

OK. Log says:
Sendmail: /usr/sbin/sendmail -i [email protected] [email protected] [email protected] [email protected]
From: cclauss <[email protected]>
To: [email protected]
Cc: [email protected],
	[email protected],
	[email protected]
Subject: On Python 3 pull request
Date: Fri, 19 Jul 2019 23:19:25 +0200
Message-Id: <[email protected]>
X-Mailer: git-send-email 2.22.0
In-Reply-To: <[email protected]>
References: <[email protected]>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

Result: OK

[cclauss]

$ cat ./note_to_git.txt

Subject: On Python 3 pull request
Hello Junio C Hamano,

  Thanks for your kind words.  Also thanks for modifying the signature
line to match the cover letter.  That is the correct edit to make.
I also agree with your rewording on print() and reduce().  However, on
input(), I must point out that python-future.org is assuming the
addition of a new dependency (pip install future).  Given the universal
deployment of git, I have chosen not to add a new production dependency
in this case.  We will do it the old fashioned way via try/except.

    - Use `input()` on Python 3 and `raw_input()` on Python 2 because
      they provide equivalent functionality.

On the flake8 test selection, this PR does _not_ focus on "_style
violations_" (the majority of flake8 error codes that
[__python/black__](https://github.com/python/black) can autocorrect).
Instead these tests are focus on runtime safety and correctness:
* E9 tests are about Python syntax errors usually raised because flake8
can not build an Abstract Syntax Tree (AST).  Often these issues are a
sign of unused code or code that has not been ported to Python 3.
 These would be compile-time errors in a compiled language but in a
dynamic language like Python they result in the script halting/crashing
on the user.
* F63 tests are usually about the confusion between identity and
equality in Python.  Use ==/!= to compare str, bytes, and int literals
is the classic case.  These are areas where __a == b__ is True but
__a is b__ is False (or vice versa).
* F7 tests logic errors and syntax errors in type hints
* F82 tests are almost always _undefined names_ which are usually a
sign of a typo, missing imports, or code that has not been ported to
Python 3.  These also would be compile-time errors in a compiled
language but in Python a __NameError__ is raised which will halt/crash
the script on the user.

On sustained maintenance of the flake8 tests, flake8 is an Open Source
project like git is so I can not make any guarantees that future
changes will not be made in that codebase,  However, flake8  is a very
stable project which is broadly used so these codes should continue to
run the same tests.  As discussed above, these tests have been
carefully selected to find only "show stopper" errors that effect
runtime safety or program correctness.  We are not enabling many
flake8 errors codes: https://lintlyci.github.io/Flake8Rules

Given all of this, is this pull request OK in its current form or does
the content or commit message need to be modified?

[gitgitgadget]

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Fri, Jul 19, 2019 at 07:18:55AM -0700, cclauss via GitGitGadget wrote:
> Several things were changed between Python 2 and Python 3.
> There are a few Python 3 incompatibilities to work on.
> Here we are making changes to make the code run on both Py2 and Py3.
> We are doing this because the end of life of Python 2 is in 167 days.
> We are using print() function because legacy print statements are syntax
> errors on Py3.
> reduce() was moved in Python 3 and raw_input() was removed so we make
> changes to avoid NameErrors being raised at runtime.
> We are also putting flake8 lint tests in place on Travis CI to avoid
> any backsliding on future pull requests.

It seems to me that this patch does too many things at once, and
perhaps it would be better to split it into a couple of smaller
patches that do only one thing, e.g.:

  - use print function instead of statement in 'hg-to-git' (which
    constitutes the bulk of this patch),
  - do the same in 'contrib/fast-import/import-zips.py'
  - import 'reduce' and fix 'raw_input' in 'git-p4'
  - and once all that is done and the linter runs clean, add the
    linter job to Travis CI.

This would ease the job of the reader, now and in the future, and it
would better stand out that ...

> diff --git a/git-p4.py b/git-p4.py
> index 3991e7d1a7..9faee25db2 100755
> --- a/git-p4.py
> +++ b/git-p4.py

> @@ -3968,6 +3970,7 @@ def renameBranch(self, branch_name):
>                  break
>  
>          if not found:
> +            sync = P4Sync()

... this change is not mentioned in the commit message.

Is this something the linter complains about?
It doesn't look like a Python 2 vs. 3 compatibility fix to me, so it
might deserve a dedicated patch.

Cc-ing Luke for this bit.

>              sys.exit("gave up trying to rename existing branch {0}".format(sync.branch))
>  
>      def findLastP4Revision(self, starting_point):
> -- 
> gitgitgadget

[dscho]

Did this get to the other side?

I don't think it did, given the thread at https://public-inbox.org/git/[email protected]/#r.

My guess is that the note_to_git.txt file did not look enough like an mbox... It should probably look more like the output of git -p format-patch --stdout -1, in particular the empty line between the header and the body. I think.

[cclauss]

The F63 instances will raise SyntaxWarnings on Python >= 3.8 so it is best to fix them now. https://docs.python.org/3.8/whatsnew/3.8.html#porting-to-python-3-8

[gitgitgadget]

This branch is now known as hb/hg-to-git-py3.

[gitgitgadget]

This patch series was integrated into pu via git@bc9ab23.

[gitgitgadget]

This patch series was integrated into pu via git@fff2f44.

[gitgitgadget]

This patch series was integrated into pu via git@1bfaea4.

[gitgitgadget]

This patch series was integrated into pu via git@6c4f114.

[gitgitgadget]

This patch series was integrated into next via git@28f7e9b.

[gitgitgadget]

This patch series was integrated into pu via git@6e5f53d.

[gitgitgadget]

This patch series was integrated into pu via git@7e8d0b2.

[gitgitgadget]

This patch series was integrated into pu via git@1c48c47.

[gitgitgadget]

This patch series was integrated into pu via git@4ee8c8f.

[gitgitgadget]

This patch series was integrated into pu via git@8f53fe1.

[gitgitgadget]

This patch series was integrated into next via git@8f53fe1.

[gitgitgadget]

This patch series was integrated into master via git@8f53fe1.

[gitgitgadget]

Closed via 8f53fe1.

[dscho]

Whoa. I think GitGitGadget mistook a related patch for superseding this here PR.

@cclauss would you mind rebasing and re-submitting?

[gitgitgadget]

Closed via 8f53fe1.

[dscho]

I guess I cannot re-open... My advice to rebase and re-submit still stands, although this should be submitted as a fresh, new GitGitGadget PR, I guess...