-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
Add lint-v2
rule and implement it for Python with black.
#8452
Add lint-v2
rule and implement it for Python with black.
#8452
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -16,3 +16,5 @@ def register_options(cls, register): | |||
super().register_options(register) | |||
register('--config', advanced=True, type=file_option, fingerprint=True, | |||
help="Path to formatting tool's config file") | |||
register('--check', advanced=True, type=bool, fingerprint=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long-term, there's an interesting question of direction here.
Historically, we had the lint
goal, which is basically fmt-without-formatting
- in the future, would we like:
- A
lint
goal alongside thefmt
goal? - A
fmt
-wide flag called something like--check
- A per-formatter flag called
--check
I suspect long-term we want 1 or 2, and we should work that out before we name the v2 console_rule fmt
, but while this is experimental until then, this works :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pattern so far has been to have ./pants lint
check for things like this, rather than to have ./pants fmt
with whitelisted options.
The reasoning for this is that ./pants lint
"never has sideeffects", while ./pants fmt
obviously might. And as more side-effecting tasks are added to fmt
, the chances of forgetting to add one of these "disable sideffects" options increases.
So as with lint
/fmt
in v1, would recommend using the existing "reformat this code" logic under a new lint-v2
rule (or under
pants/src/python/pants/backend/project_info/rules/source_file_validator.py
Lines 210 to 215 in 138746a
# TODO: Switch this to `lint` once we figure out a good way for v1 tasks and v2 rules | |
# to share goal names. | |
@console_rule | |
def validate( | |
console: Console, hydrated_targets: HydratedTargets, validate_options: Validate.Options | |
) -> Validate: |
@rule
comparing the resulting digest to the target's input digest to see whether the lint failed.
f4f2091
to
82788a9
Compare
--check
argument to the black subsystem.lint-v2
rule and implement it for Python with black.
The code here looks good to me, but I'm still unsure of the long-term direction; I think that ignoring history, I probably would design these goals as |
The issue with My feeling though is that all realistic cases will want to run "all of the lints" (assuming that they are fast enough). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
But please wait for Benjy to chime in on the fmt --check
vs lint
distinction.
Thanks - code looks good. After some thought, I come down on the side of putting this in a Another way to think of it is that "formatter in check mode" is an implementation detail of a lint rule. |
+1 to this being a lint goal. (Haven’t checked the code at all.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Two minor things :)
results = yield [ | ||
Get(LintResult, FmtTarget, target.adaptor) | ||
for target in targets | ||
# @union assumes that all targets passed implement the union, so we manually |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #8368 - you can now inject a UnionMembership
to the rule, and call union_membership.is_member(FmtTarget, target.adaptor)
(At some point we'll probably want to also create a LintTarget
Union
, but piggybacking off of FmtTarget
is probably sufficient until we either add a non-fmt lint, or rename this goal to be lint
instead of lint-v2
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will address in a separate PR to hopefully get this one merged today (I'm also dragging #8454 which is based on top of this, so would be keen on iterating fast if possible)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done here: #8490
src/python/pants/rules/core/lint.py
Outdated
if result.stderr: | ||
console.print_stderr(result.stderr) | ||
if result.exit_code != 0: | ||
yield Lint(result.exit_code) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A console rule should only yield an output exactly one time.
Can you make your test_black_lint_should_fail_but_not_format_python_code
test lint two incorrectly-formatted things? You should then see an error (the fix for which is probably to accumulate 0 or last-non-0 exit code, and yield that at the end)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a couple of tests: lint multiple files and lint multiple targets. With the latter, I could reproduce the issue you're describing (we don't report all failing targets). I fixed it as you described.
82788a9
to
e7877b1
Compare
Just a comment to be explicit: the reasoning for |
f6e277e
to
f4519b9
Compare
f4519b9
to
0053383
Compare
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.
0053383
to
7744b21
Compare
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.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.