Skip to content

Travis CI: Lint for Python syntax errors and undefined names #290

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Travis CI: Lint for Python syntax errors and undefined names #290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ matrix:
compiler:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

script: ci/test-documentation.sh
after_failure:
- env: jobname=LintPython
language: python
before_install: pip install flake8
script: flake8 . --count --select=E9,F63,F72,F82 --show-source --statistics

before_install: ci/install-dependencies.sh
script: ci/run-build-and-tests.sh
Expand Down
3 changes: 2 additions & 1 deletion contrib/fast-import/import-zips.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
## python import-zips.py *.zip
## git log --stat import-zips

from __future__ import print_function
from os import popen, path
from sys import argv, exit, hexversion, stderr
from time import mktime
Expand All @@ -19,7 +20,7 @@
exit(1)

if len(argv) < 2:
print 'usage:', argv[0], '<zipfile>...'
print('usage:', argv[0], '<zipfile>...')
exit(1)

branch_ref = 'refs/heads/import-zips'
Expand Down
47 changes: 24 additions & 23 deletions contrib/hg-to-git/hg-to-git.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
along with this program; if not, see <http://www.gnu.org/licenses/>.
"""

from __future__ import print_function
import os, os.path, sys
import tempfile, pickle, getopt
import re
Expand All @@ -42,7 +43,7 @@

def usage():

print """\
print("""\
%s: [OPTIONS] <hgprj>

options:
Expand All @@ -54,7 +55,7 @@ def usage():

required:
hgprj: name of the HG project to import (directory)
""" % sys.argv[0]
""" % sys.argv[0])

#------------------------------------------------------------------------------

Expand Down Expand Up @@ -104,22 +105,22 @@ def getgitenv(user, date):
if state:
if os.path.exists(state):
if verbose:
print 'State does exist, reading'
print('State does exist, reading')
f = open(state, 'r')
hgvers = pickle.load(f)
else:
print 'State does not exist, first run'
print('State does not exist, first run')

sock = os.popen('hg tip --template "{rev}"')
tip = sock.read()
if sock.close():
sys.exit(1)
if verbose:
print 'tip is', tip
print('tip is', tip)

# Calculate the branches
if verbose:
print 'analysing the branches...'
print('analysing the branches...')
hgchildren["0"] = ()
hgparents["0"] = (None, None)
hgbranch["0"] = "master"
Expand Down Expand Up @@ -155,7 +156,7 @@ def getgitenv(user, date):
hgbranch[str(cset)] = "branch-" + str(cset)

if not hgvers.has_key("0"):
print 'creating repository'
print('creating repository')
os.system('git init')

# loop through every hg changeset
Expand All @@ -180,27 +181,27 @@ def getgitenv(user, date):
os.write(fdcomment, csetcomment)
os.close(fdcomment)

print '-----------------------------------------'
print 'cset:', cset
print 'branch:', hgbranch[str(cset)]
print 'user:', user
print 'date:', date
print 'comment:', csetcomment
print('-----------------------------------------')
print('cset:', cset)
print('branch:', hgbranch[str(cset)])
print('user:', user)
print('date:', date)
print('comment:', csetcomment)
if parent:
print 'parent:', parent
print('parent:', parent)
if mparent:
print 'mparent:', mparent
print('mparent:', mparent)
if tag:
print 'tag:', tag
print '-----------------------------------------'
print('tag:', tag)
print('-----------------------------------------')

# checkout the parent if necessary
if cset != 0:
if hgbranch[str(cset)] == "branch-" + str(cset):
print 'creating new branch', hgbranch[str(cset)]
print('creating new branch', hgbranch[str(cset)])
os.system('git checkout -b %s %s' % (hgbranch[str(cset)], hgvers[parent]))
else:
print 'checking out branch', hgbranch[str(cset)]
print('checking out branch', hgbranch[str(cset)])
os.system('git checkout %s' % hgbranch[str(cset)])

# merge
Expand All @@ -209,7 +210,7 @@ def getgitenv(user, date):
otherbranch = hgbranch[mparent]
else:
otherbranch = hgbranch[parent]
print 'merging', otherbranch, 'into', hgbranch[str(cset)]
print('merging', otherbranch, 'into', hgbranch[str(cset)])
os.system(getgitenv(user, date) + 'git merge --no-commit -s ours "" %s %s' % (hgbranch[str(cset)], otherbranch))

# remove everything except .git and .hg directories
Expand All @@ -233,12 +234,12 @@ def getgitenv(user, date):

# delete branch if not used anymore...
if mparent and len(hgchildren[str(cset)]):
print "Deleting unused branch:", otherbranch
print("Deleting unused branch:", otherbranch)
os.system('git branch -d %s' % otherbranch)

# retrieve and record the version
vvv = os.popen('git show --quiet --pretty=format:%H').read()
print 'record', cset, '->', vvv
print('record', cset, '->', vvv)
hgvers[str(cset)] = vvv

if hgnewcsets >= opt_nrepack and opt_nrepack != -1:
Expand All @@ -247,7 +248,7 @@ def getgitenv(user, date):
# write the state for incrementals
if state:
if verbose:
print 'Writing state'
print('Writing state')
f = open(state, 'w')
pickle.dump(hgvers, f)

Expand Down
3 changes: 3 additions & 0 deletions git-p4.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
unicode = str
bytes = bytes
basestring = (str,bytes)
raw_input = input
from functools import reduce
else:
# 'unicode' exists, must be Python 2
str = str
Expand Down Expand Up @@ -3968,6 +3970,7 @@ def renameBranch(self, branch_name):
break

if not found:
sync = P4Sync()
sys.exit("gave up trying to rename existing branch {0}".format(sync.branch))

def findLastP4Revision(self, starting_point):
Expand Down