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.

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

Run pythonstyle under the appropriate interpreter. #6618

Merged
merged 3 commits into from Oct 12, 2018

Conversation

Projects
None yet
4 participants
@jsirois
Copy link
Member

jsirois commented Oct 11, 2018

This breaks the pythonstyle lint task up into a task frontend and pex
backend. This splitting allows the backend to be run under any
interpreter Pants supports independent of the interpreter currently
running Pants.

Fixes #5764

@jsirois jsirois requested review from Eric-Arellano , CMLivingston and stuhood Oct 11, 2018

@Eric-Arellano
Copy link
Contributor

Eric-Arellano left a comment

This PR is pretty big so I'll review tomorrow.

For context, this looks to build off of #6274. I had made a bunch of fixes to allow us to parse both Python 2 and Python 3 ASTs. Didn't realize we weren't actually using it on Py3, though. As I understand, John's PR here allows us to use Py3 and determines which AST to use based off of the interpreter. (John, correct me if wrong - I only quickly glanced).

John, do you mind flagging lines where you made major changes and/or you'd appreciate closer review?

Thanks!

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Oct 11, 2018

John's PR here allows us to use Py3 and determines which AST to use based off of the interpreter.

Yesish. The emphasis is a bit odd in a similar fashion to the wording in #5764. The linter simply becomes a standard python tool now independent from pants (with some conditional behavior you introduced in #6274 baked in). We execute the the tool with an interpreter appropriate to the sources we hand it to lint. That interpreter of course has as a part of its own stdlib the right ast.

John, do you mind flagging lines where you made major changes and/or you'd appreciate closer review?

The change mainly consists of:

  1. [key] splitting the task (contrib/python/src/python/pants/contrib/python/checks/tasks/checkstyle/checker.py) into task frontend and pex backend and handling plugin option handoff with json blobs from the subsystems:

  2. [mechanical] moving lint plugins contrib/python/src/python/pants/contrib/python/checks/{tasks/checkstyle => checker}

  3. [mechanical] killing most lint plugin subsystems in favor of a generic factory:

  4. [mechanical] moving lint plugin tests contrib/python/tests/python/pants_test/contrib/python/checks/{tasks/checkstyle => checker}

The only other interesting bit besides 1 above that I'd like feedback on is the use of PANTS_DEV as a means to flip to a local mode where the lint pex is built straight from the sources in the Pants repo vs the normal mode of creating the lint tool pex from a new wheel we (will) release. Both paths are tested in CI. We run lint against our own sources both loose locally (pants script PANTS_DEV=1 and via a pants.pex in ci.sh with PANTS_DEV=1) and via a wheel generated in the task unit test. It seems this system could use more formalization in follow-ups if acceptable in much the same way as jvm tool bootstrapping.

jsirois added a commit to jsirois/pants that referenced this pull request Oct 11, 2018

Introduce factory_dict.
This generates values from keys when no entry is present, a feature
you might guess collections.defaultdict would support by passing keys
through to default_factory functions.

Update products to use this.
PR pantsbuild#6618 will also pull it in or else it will be used in a follow-up.

@jsirois jsirois referenced this pull request Oct 11, 2018

Merged

Introduce factory_dict. #6622

jsirois added a commit to jsirois/pants that referenced this pull request Oct 11, 2018

Introduce factory_dict.
This generates values from keys when no entry is present, a feature
you might guess collections.defaultdict would support by passing keys
through to default_factory functions.

Update products to use this.
PR pantsbuild#6618 will also pull it in or else it will be used in a follow-up.

jsirois added some commits Oct 11, 2018

Run pythonstyle under the appropriate interpreter.
This breaks the pythonstyle lint task up into a task frontend and pex
backend. This splitting allows the backend to be run under any
interpreter Pants supports and thus support linting python 3 code for
example.

Fixes #5764

@jsirois jsirois force-pushed the jsirois:issues/5764 branch from ba57d20 to 40ad063 Oct 11, 2018

@CMLivingston
Copy link
Contributor

CMLivingston left a comment

Thanks for making this change and for adding the tests. I can take this over and incorporate my changes.

jsirois added a commit that referenced this pull request Oct 12, 2018

Introduce factory_dict. (#6622)
This generates values from keys when no entry is present, a feature
you might guess collections.defaultdict would support by passing keys
through to default_factory functions.

Update products to use this.
PR #6618 will also pull it in or else it will be used in a follow-up.

@jsirois jsirois merged commit 0a39ffd into pantsbuild:master Oct 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jsirois jsirois deleted the jsirois:issues/5764 branch Oct 12, 2018

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Oct 12, 2018

I may have pulled the trigger too fast. @Eric-Arellano if you do have feedback I'll happily follow-up and address.

@Eric-Arellano
Copy link
Contributor

Eric-Arellano left a comment

Looks good! Feel free 100% to ignore the first suggestion. Other two are nothing more than comments.

@@ -146,7 +148,7 @@ def from_statement(cls, statement, filename='<expr>'):

blob = '\n'.join(lines).encode('utf-8')
tree = cls._parse(blob, filename)
return cls(blob, tree, filename)
return cls(blob, tree, None, filename)

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Oct 12, 2018

Contributor

This None is a little confusing what it refers to (at least from the GitHub UI, which doesn't show the whole surrounding context). Keyword args would help.

If there are enough other changes I suggest in later files, then maybe worth changing. Otherwise feel free to ignore this.

This comment has been minimized.

@jsirois

jsirois Oct 12, 2018

Member

Agreed - fixed.

This comment has been minimized.

@jsirois
@@ -47,6 +47,10 @@ def get_error_code(cls, message):
class PyflakesChecker(CheckstylePlugin):
"""Detect common coding errors via the pyflakes package."""

@classmethod
def name(cls):
return 'pyflakes'

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Oct 12, 2018

Contributor

Nice, I like this change.

argfile.write('--{}-options={}\n'.format(plugin_subsystem.plugin_type().name(),
options_blob))
argfile.write('\n'.join(sources))
argfile.close()

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Oct 12, 2018

Contributor

This line surprises me because of the with statement. Is it necessary?

This comment has been minimized.

@jsirois

jsirois Oct 12, 2018

Member

At least a flush is. We're about to hand the now written temp file off to a subprocess to read in. Here the with statement nets us an unlink (of course a close happens too, but the mental model should be that the resource is the file not the file handle as it would be using the open context manager).

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Oct 12, 2018

This is awesome. Thanks for diving in here John!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment