It is safer to pylint on Python 3 than Python 2 by cclauss · Pull Request #5727 · certbot/certbot

cclauss · 2018-03-14T16:49:53Z

Syntax Errors will be caught by pylint running on Python 3 but not when running on Python 2:

print "Hello"
0L
0755
except Exception, e
etc...

ohemorange

Hey @cclauss, it would be absolutely fantastic if you could work on getting our pylint tests running in python 3! It's going to take some work though, because they don't currently pass when run with python 3.

Here's what's up: changing .travis.yml has travis set up a virtual env with the python specified (what you just changed to 3.6). But our tox.ini file specifies which python to actually run the tests with. That's currently set to python2.7, and should be changed to python3 to get it to actually use a version of python3 to run tests (it will be 3.6 in travis as specified, and whatever version of 3 people have locally when run not on travis).

The lint tests should work with any version of python 3 we support. Not sure about others, but they're definitely failing on python3.5.

Based on my own brief investigation, there's a bug in the version of pylint we use that makes it fail on some set of versions of python including 3.5. This could be fixed by upgrading pylint, but it's currently pinned to an older version because of reasoning you can find here: #289

I would love it if you could investigate this issue so we could resolve it properly, but if it's more work than you thought you'd be getting into let me know and I'll close the PR. Thanks!

cclauss · 2018-04-18T08:06:15Z

The really strange case of python3 -m pylint import_os.py...

$ git clone https://github.com/certbot/certbot
$ cd certbot
$ cat > import_os.py

import os
<Ctrl-D>

$ python3 -m pylint import_os.py

internal error with sending report for module ['import_os.py']
The concept of slots is undefined for old-style classes.

$ cp import_os.py ..
$ cd ..
$ python3 -m pylint import_os.py

Pylint works as expected

Conclusion: There is not a problem with your Python files, there is a problem with your error reporting system. If you try the same trick with import_sys.py things work so there is really something about "os" like perhaps there is a file named os.py lurking around somewhere or something.

cclauss · 2018-04-18T19:24:12Z

@ohemorange This is ready for your review. There were several head scratchers in this one...

.travis.yml: Just upgrade to Python 3.6 with allow_failures so the pylint issues can be fixed
linter_plugin.py:
- The file was missing a pass statement ~~so it was not valid Python~~
- The pylint plugin API has changed so nodes are not longer required
- Comment out the cls.slots() code as they no longer exists
setup.py: Remove an unused import (Remove unneeded sys import #5873) and unpeg the imports
tox.ini:
- Upgrade to Python 3
- Reinstall astroid and pylint -- without this re-installation, bad things happen :-(
  - This was the hardest issue to find and it is counter-intuitive but essential
- ~~Temporarily end the pytest line with ; true so that Travis does not rerun failing tests~~
  - ~~Once all pytest issues have been fixed, the trailing ; true should be removed.~~

ohemorange

Nice sleuthing, cclauss! Thanks for investigating this.

Looks like with later versions of things, we can go ahead and remove linter_plugin.py entirely by deleting that file and changing load-plugins=linter_plugin to load-plugins= in .pylintrc, and removing include linter_plugin.py from MANIFEST.in.

Since we rely on clean lint results for new code, I'm not comfortable merging a change into master that allows lint to fail. How do you feel about attacking the (quite lengthy but probably straightforward) list of lint failures that now show up in travis? Those can be done in separate PRs, and we can merge this PR (minus allow_failures) once we're back to a clean lint.

cclauss · 2018-04-19T07:47:34Z

we rely on clean lint results for new code

Not really. Pylint dirt is currently being swept under the rug.

This PR demonstrates that for a while now Pylint has been running improperly both locally or on Travis because of linter_plugin.py. Once that is fixed, then Pylint errors are properly reported.

The PR has now reverted to running Pylint on Python 2 and the repo currently contains 90+ Python files that generate Pylint errors. I am not inclined to fix all those files but it might make a nice set of tasks for first time contributors.

When linter_plugin.py is deleted then the apache_compat and nginx_compat test runs fail. So linter_plugin.py is fixed to be valid Python and the cls.slots() bit commented out. .pylintrc is modified to no longer load the plugin.

ohemorange

Sounds like you might not have the time to clean the lint errors. We have a hackathon coming up, so maybe we can address many of those issues and get the new way of doing things merged afterwards. Thanks again for your hard work here!

ohemorange · 2018-04-26T22:52:51Z

tox.ini

 # continue, but tox return code will reflect previous error
 commands =
    {[base]install_packages}
+    pip install --upgrade astroid pylint  # required!


This shouldn't actually be required; developers might just have to remove and reinstall the venv.

Try taking it out and see what breaks.

tools/dev_constraints.txt should also be updated

Verify no conficts when working on old PR certbot#5727

bmw · 2018-05-30T20:09:04Z

@ohemorange, are you happy with this PR besides fixing the new linting failures?

ohemorange · 2018-06-01T20:53:30Z

@bmw no, there's still an unaddressed review comment that I'd like to see cleared up.

bmw · 2018-06-01T21:03:29Z

OK. The fix here is to remove the line commented on at #5727 (comment) and update the pinned versions in tools/dev_constraints.txt.

@sydneyli, perhaps @jepayne1138 can do this in #6008?

ohemorange · 2018-09-12T22:45:48Z

Closing this in favor of #6008. @cclauss if you can make the change described at #5727 (comment), then we'll reopen to finish it up here. Thanks for all the great work you've done so far!

schoen added area: code health area: python area: testing labels Mar 20, 2018

ohemorange requested changes Apr 17, 2018

View reviewed changes

cclauss force-pushed the patch-2 branch from 84ee320 to c44960a Compare April 17, 2018 20:05

cclauss force-pushed the patch-2 branch 2 times, most recently from ad297dd to f7b1d1a Compare April 18, 2018 19:10

cclauss force-pushed the patch-2 branch 9 times, most recently from bc0aaf6 to 85ac3af Compare April 18, 2018 22:27

ohemorange requested changes Apr 19, 2018

View reviewed changes

cclauss force-pushed the patch-2 branch 6 times, most recently from 32a0220 to aca4bf5 Compare April 19, 2018 06:56

Safer to pylint on Python 3

2991813

cclauss force-pushed the patch-2 branch from aca4bf5 to 2991813 Compare April 19, 2018 07:39

ohemorange requested changes Apr 26, 2018

View reviewed changes

ohemorange mentioned this pull request Apr 26, 2018

Address linter issues from using upgraded pylint #5904

Closed

ohemorange mentioned this pull request May 2, 2018

Check_untyped_defs in mypy with clean output for acme #5874

Merged

jepayne1138 added a commit to jepayne1138/certbot that referenced this pull request May 14, 2018

Merge branch 'master' into pylint

326cda0

Verify no conficts when working on old PR certbot#5727

bmw mentioned this pull request May 30, 2018

Update Pylint to 1.8.4 #6008

Merged

ohemorange closed this Sep 12, 2018

cclauss deleted the patch-2 branch September 12, 2018 22:56

bmw mentioned this pull request Nov 12, 2019

Run our coverage and lint tests on Python 3 #7550

Closed

Uh oh!

Conversation

cclauss commented Mar 14, 2018

Uh oh!

ohemorange left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cclauss commented Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cclauss commented Apr 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ohemorange left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cclauss commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ohemorange left a comment

Choose a reason for hiding this comment

Uh oh!

ohemorange Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

cclauss Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

ohemorange Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

bmw commented May 30, 2018

Uh oh!

ohemorange commented Jun 1, 2018

Uh oh!

bmw commented Jun 1, 2018

Uh oh!

ohemorange commented Sep 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ohemorange left a comment •

edited

Loading

cclauss commented Apr 18, 2018 •

edited

Loading

cclauss commented Apr 18, 2018 •

edited

Loading

ohemorange left a comment •

edited

Loading

cclauss commented Apr 19, 2018 •

edited

Loading