"Unify" `fmt` and `lint` rules for formatters #14903

thejcannon · 2022-03-24T12:58:00Z

(proposal: https://docs.google.com/document/d/1KqrPP6VVi-Kq8EUqfo2IFffaHxdysCMPwrInoewM-LM/edit?usp=sharing)

This PR makes changes to lint.py to create FmtRequests for the purpose of linting, coercing the FmtResult into LintResult.

The core changes:

lint.py now constructs FmtRequests in addition to the normal lint request types, and coerces the FmtResult into a LintResult
FmtResult's message is now a simple tabbed list of files that changed. This will be used for both fmt and lint.
The FmtResult stdout and stderr are now debug logged. Unfortunately there's some out-of-order logging, but I think this is OK. Most users don't care.

The formatter plugin changes:

Removing the lint rules for formatters 🎉
Which in turn removes the check_only which elides the SetupRequest in each formatter
Elided some argv generation

This does not fix the issue pointed out in #14489 we can solve that later.

Beofre and after (using 4 cores)

Benchmark 1: ./pants --process-execution-local-parallelism=4 --no-pantsd --no-local-cache fmt lint ::
  Time (mean ± σ):     40.734 s ±  0.456 s    [User: 106.734 s, System: 13.017 s]
  Range (min … max):   40.375 s … 41.519 s    5 runs

Benchmark 1: ./pants --process-execution-local-parallelism=4 --no-pantsd --no-local-cache fmt lint ::
  Time (mean ± σ):     34.397 s ±  0.407 s    [User: 74.738 s, System: 9.009 s]
  Range (min … max):   33.987 s … 35.071 s    5 runs

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

thejcannon · 2022-03-24T13:03:28Z

Commits are useful in order

thejcannon · 2022-03-24T13:04:16Z

src/python/pants/backend/go/lint/gofmt/rules.py

+        if request.prior_formatter_result is None
+        else request.prior_formatter_result


In a future change I think I'll refactor so this isn't needed. It's so confusing 😵

Please!! I can't remember why this was necessary. But it's so copy pasta now.

TL;DR Because these rules run for fmt (in which case the formatter should run on the prior result) and lint (in which case there is no "prior formatter result")

thejcannon · 2022-03-24T13:31:11Z

Also, it'd be a breaking user change so likely not worth it, but I wish the formatters were in a fmt directory in-repo 😞

thejcannon · 2022-03-24T13:35:36Z

src/python/pants/core/goals/lint.py

+        if isinstance(batch_results, FmtResult):
+            return (
+                LintResult(
+                    1 if batch_results.did_change else 0,


It's a bummer we have to invent an error code here 😒

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

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

stuhood

Thanks a lot! This is a great cleanup.

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

thejcannon · 2022-03-28T19:27:56Z

As a follow-up we can issue a deprecation warning if our lint and fmt requests share a type (e.g. remove your lint code)

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

thejcannon · 2022-03-28T19:38:29Z

As a follow-up we can issue a deprecation warning if our lint and fmt requests share a type (e.g. remove your lint code)

Oh hey someone has already done that, indirectly 😂 AmbiguousRequestNamesError.

Eric-Arellano · 2022-03-30T00:32:22Z

This does not fix the issue pointed out in #14489 we can solve that later.

How so? I don't think this is enough to block on, but is it solvable?

thejcannon · 2022-03-30T00:34:21Z

This does not fix the issue pointed out in #14489 we can solve that later.

How so? I don't think this is enough to block on, but is it solvable?

Well this actually perpetuates that issue into lint, as the tools will be trying to write to disc for lint.

It'll need to be addressed, but my approach would just be to give a fallback message about the formatter failing to format a file, which implies a file would fail lint anyways. E.g. black attempted to write to disk but failed. Please run ./pants fmt to fix formatting.

Eric-Arellano

Amazing work. Not only really good code, but you did an excellent job with the proposal & google doc etc. So exciting 🎉

Eric-Arellano · 2022-03-30T00:34:36Z

src/python/pants/backend/go/lint/gofmt/rules.py

+        if request.prior_formatter_result is None
+        else request.prior_formatter_result


Please!! I can't remember why this was necessary. But it's so copy pasta now.

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

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

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

Co-authored-by: Eric Arellano <[email protected]>

thejcannon added 3 commits March 23, 2022 19:51

core goal changes

d283841

additional lint changes

7d6bc3a

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

rule changes

2812259

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

thejcannon added category:user api change category:plugin api change category:performance labels Mar 24, 2022

thejcannon commented Mar 24, 2022

View reviewed changes

test fallout

6373e69

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

thejcannon requested review from Eric-Arellano and stuhood March 24, 2022 14:12

thejcannon removed category:user api change category:performance labels Mar 24, 2022

thejcannon added 2 commits March 24, 2022 12:04

yapf arg

fa72e9f

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

fix with .get

3141eea

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano mentioned this pull request Mar 25, 2022

Add Protobuf formatting using buf format #14907

Merged

thejcannon commented Mar 28, 2022

View reviewed changes

src/python/pants/core/goals/fmt.py Show resolved Hide resolved

stuhood approved these changes Mar 28, 2022

View reviewed changes

src/python/pants/core/goals/fmt.py Outdated Show resolved Hide resolved

src/python/pants/core/goals/fmt.py Show resolved Hide resolved

thejcannon added 2 commits March 28, 2022 12:52

Merge branch 'main' into fmt-in-lint

e91be8c

protobuf

abc0a61

test changes

86002f9

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano approved these changes Mar 30, 2022

View reviewed changes

comments

1967f52

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Eric-Arellano reviewed Mar 30, 2022

View reviewed changes

src/python/pants/core/goals/fmt.py Outdated Show resolved Hide resolved

Eric-Arellano approved these changes Mar 30, 2022

View reviewed changes

Update src/python/pants/core/goals/fmt.py

a61102d

Co-authored-by: Eric Arellano <[email protected]>

thejcannon enabled auto-merge (squash) March 30, 2022 13:57

thejcannon merged commit c70449c into pantsbuild:main Mar 30, 2022

thejcannon deleted the fmt-in-lint branch March 30, 2022 15:04

This was referenced Apr 2, 2022

[internal] Pass prior_formatter_result in lintto FmtRequest #14984

Merged

Add a helpful hint to lint if a formatter fails #15048

Merged

Eric-Arellano mentioned this pull request May 14, 2022

update linters to not capture outputs when only checking formatting #13819

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"Unify" `fmt` and `lint` rules for formatters #14903

"Unify" `fmt` and `lint` rules for formatters #14903

thejcannon commented Mar 24, 2022

thejcannon commented Mar 24, 2022

thejcannon Mar 24, 2022

Eric-Arellano Mar 30, 2022

thejcannon Mar 30, 2022

thejcannon commented Mar 24, 2022

thejcannon Mar 24, 2022

stuhood left a comment

thejcannon commented Mar 28, 2022

thejcannon commented Mar 28, 2022

Eric-Arellano commented Mar 30, 2022

thejcannon commented Mar 30, 2022

Eric-Arellano left a comment

Eric-Arellano Mar 30, 2022

		if request.prior_formatter_result is None
		else request.prior_formatter_result

"Unify" fmt and lint rules for formatters #14903

"Unify" fmt and lint rules for formatters #14903

Conversation

thejcannon commented Mar 24, 2022

thejcannon commented Mar 24, 2022

thejcannon Mar 24, 2022

Choose a reason for hiding this comment

Eric-Arellano Mar 30, 2022

Choose a reason for hiding this comment

thejcannon Mar 30, 2022

Choose a reason for hiding this comment

thejcannon commented Mar 24, 2022

thejcannon Mar 24, 2022

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

thejcannon commented Mar 28, 2022

thejcannon commented Mar 28, 2022

Eric-Arellano commented Mar 30, 2022

thejcannon commented Mar 30, 2022

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano Mar 30, 2022

Choose a reason for hiding this comment

"Unify" `fmt` and `lint` rules for formatters #14903

"Unify" `fmt` and `lint` rules for formatters #14903