Add lint-v2 implementation for python with black, and script to be used in pre-commit hook #8553

pierrechevalier83 · 2019-10-30T14:24:49Z

Problem

In the near future, we will want to add a pre-commit hook that helps us remember to automatically format our code with black.
Currently, the version of black that's integrated in pants will always modify the user's code, which may not be desirable in a pre_commit hook situation.

Solution

Add a lint-v2 rule which uses black with the --check flag to highlight unformatted files without actually modifying them.
Add a script which runs the lint-v2 rule on all targets touched since upstream
Use UnionMembership to reduce coupling in fmt and lint v2 rules

Result

See integration tests for examples: User can call ./pants lint-v2 .... The command will fail if the files would be formatted, but won't rewrite the files.

This is a prerequisite for ultimately adding a pre-commit hook that behaves similarly to our isort pre-commit hook (warns the user and lets them actually modify their code once they are ready for it) * Create a top level `lint-v2` rule; * Implement `lint_with_black` which implements that rule for python; * Deduplicate `lint_with_black` with `fmt_with_black`; * Make use of `FallibleExecuteProcessRequest` to make lint output less spammy.

Now we introduced a lint goal which takes the same kind of targets as an input, it makes sense to use a more general name.

The issue that was link before isn't obviously related to the issue. The actual problem is `TargetAdaptor.sources` throwing an exception.

pierrechevalier83 · 2019-10-30T14:30:40Z

This PR is an aggregate of #8452, #8454 and #8490

I wanted to merge them in order to preserve a bit more git history, but I haven't been able to merge even a single PR in the last 3 weeks due to a mix of CI flakyness and broken master. Reducing these PRs to one should allow me to merge them earlier (as soon as I get one single green CI).

All the code in previous PRs was already reviewed and the only unaddressed comments were higher level discussions about v2 rules on #8490 (which I don't think required action on this particular PR)

ity

i have just done a quick browse since all of these changes were already reviewed in the other 3 PRs.

stuhood

Thanks! Please open a followup PR to address the nits.

stuhood · 2019-10-30T16:21:01Z

build-support/bin/common.py

@@ -53,6 +54,17 @@ def elapsed_time() -> Tuple[int, int]:
  return elapsed_seconds // 60, elapsed_seconds % 60


+def git_merge_base() -> str:


We should follow up on this, I think: this script should not have different behaviour from our other linters, and so the fact that this function didn't exist before might indicate that it doesn't need to.

This is basically a duplicate of the one in common.sh. Eric was of the opinion that it was worth having these utilities straight in python for the python scripts. In the (very) long run we can dedup by removing all the shell scripts.

(Note that I originally wrote this script as a bash script, but python was deemed preferable as that's the direction we're trending towards for these kind of scripts)

stuhood · 2019-10-30T16:23:00Z

src/python/pants/backend/python/rules/python_fmt.py

+
+
+@rule
+def fmt_with_black(


Nit: fmt and lint don't need to actually use the --check flag: they can be completely identical, and just check whether the output snapshot matches. Maybe using --check would be slightly faster for a single run? But if you assume that the next thing someone will do after lint fails is run fmt, the impact of running exactly the same process is that it would be cached, and the two runs overall would be faster.

--check makes black output prettier diagnostics (N files would have been formatted)

We can also render those diagnostics, but in a more general way.

That's true. Possibly a bit more work, but I get why it would be desirable.

Note that this discussion is currently about black, but it's a general point that we may want to get right across the variety of formatters and linters that the v2 engine will eventually support. (Brief summary from private chat with @stuhood)

stuhood · 2019-10-30T16:24:10Z

src/python/pants/rules/core/fmt.py

@@ -27,7 +21,7 @@ class FmtResult:


 @union
-class FmtTarget:
+class TargetWithSources:


This no longer seems to mean this. Please loop back to reference #4535 here in a TODO. And given how generally it's being treated, it should likely exist in some other file, near the target API itself.

pierrechevalier83 added 6 commits October 30, 2019 10:35

Rename FmtTarget to TargetWithSources

7744b21

Now we introduced a lint goal which takes the same kind of targets as an input, it makes sense to use a more general name.

Add script to run black on targets touched by local work

96adf27

Use UnionMembership to reduce coupling in fmt and lint v2 rules

53fe4ff

Improve comment in fmt and lint rules

38800c6

The issue that was link before isn't obviously related to the issue. The actual problem is `TargetAdaptor.sources` throwing an exception.

Make two functions private in python_fmt

74a795c

This was referenced Oct 30, 2019

Add lint-v2 rule and implement it for Python with black. #8452

Closed

Add script to run black on files touched by local work #8454

Closed

Use UnionMembership to reduce coupling in fmt and lint v2 rules #8490

Closed

pierrechevalier83 changed the title ~~Pchevalier/pre black/lint v2 and script~~ Add lint-v2 implementation for python with black, and script to be used in pre-commit hook Oct 30, 2019

ity requested review from stuhood, illicitonion and ity October 30, 2019 14:33

ity approved these changes Oct 30, 2019

View reviewed changes

stuhood approved these changes Oct 30, 2019

View reviewed changes

ity merged commit 0e15a34 into pantsbuild:master Oct 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lint-v2 implementation for python with black, and script to be used in pre-commit hook #8553

Add lint-v2 implementation for python with black, and script to be used in pre-commit hook #8553

pierrechevalier83 commented Oct 30, 2019

pierrechevalier83 commented Oct 30, 2019 •

edited

Loading

ity left a comment

stuhood left a comment

stuhood Oct 30, 2019

pierrechevalier83 Oct 30, 2019

pierrechevalier83 Oct 30, 2019

stuhood Oct 30, 2019 •

edited

Loading

pierrechevalier83 Oct 30, 2019

stuhood Oct 30, 2019

pierrechevalier83 Oct 30, 2019

pierrechevalier83 Oct 30, 2019

stuhood Oct 30, 2019

		@@ -53,6 +54,17 @@ def elapsed_time() -> Tuple[int, int]:
		return elapsed_seconds // 60, elapsed_seconds % 60


		def git_merge_base() -> str:

Add lint-v2 implementation for python with black, and script to be used in pre-commit hook #8553

Add lint-v2 implementation for python with black, and script to be used in pre-commit hook #8553

Conversation

pierrechevalier83 commented Oct 30, 2019

Problem

Solution

Result

pierrechevalier83 commented Oct 30, 2019 • edited Loading

ity left a comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood Oct 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierrechevalier83 commented Oct 30, 2019 •

edited

Loading

stuhood Oct 30, 2019 •

edited

Loading