Improve output of V2 fmt and lint goals
#9710
Conversation
[ci skip-rust-tests] [ci skip-jvm-tests]
Eric-Arellano
requested review from
stuhood,
jsirois,
benjyw,
asherf,
cosmicexplorer and
gshuflin
|
I think we might want to route tool stdout to our stderr, so that everything, including our headings, go there. It's probably wrong for linters to write to stdout anyway. Could be convinced either way though. |
I think most linter can write results to some output file... |
Yes to all of these! I think these are very thoughtful.
+1. If a linter writes to stdout I'm thinking it should be something structured like json. If it's all text, I think stderr makes perfect sense. Several ways to do this:
EDIT: I really like @asherf's note above. While the right arguments to make a linter interleave output might not exist, an output file feels much more likely to be a command-line option (and we do always have the option of #9101 if we need to do one-off redirections). I like the idea of using an output file because it seems like something that'd be more reliably available as a CLI arg, but I also like the idea of (4) above, which wouldn't require modifying any command lines. |
# Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]
|
Okay, talked to John, and decided per your suggestions + his to always write to stderr. John pointed out this would better follow the Unix philosophy - users cannot pipe the output to other programs because it will be mangled with the output of other linters, so it belongs in stderr. He coined the "emoji rule": "emoji == bs for humans == stderr" 😂 |
I love this!!! |
# Delete this line to force CI to run Clippy and the Rust tests. [ci skip-rust-tests] # Delete this line to force CI to run the JVM tests. [ci skip-jvm-tests]
[ci skip-jvm-tests] [ci skip-rust-tests]
There was a problem hiding this comment.
Thanks!
As discussed offline, I feel pretty strongly that similar to with test we need to move to a place where we can render actionable (ie, failing or warning) output immediately while other things are still being computed (ie, show you the results of one linter or one-linter-for-one-target while others are still running). I believe that that means that this will need to change further: but this definitely clarifies things in the meantime, so we can cross that bridge soon.
| if result.stdout: | ||
| console.print_stdout(result.stdout) | ||
| console.print_stderr(result.stdout) |
There was a problem hiding this comment.
Moving forward (and if we think that it generalizes) we should consider whether we want to indent tool outputs by a small amount (ie, nowhere near the unbounded indentation of v1). So I think we'd have exactly two levels of indentation: the workunit containers would be un-indented, and tool outputs would be indented.
Agreed. I also strongly agree with that. I think we will end up using very similar wording to this PR, just we'll render it right away, rather than at the end. |
Changes: * Don't log with `test --debug`. * Stop explicitly separating `stdout` and `stderr` into distinct sections for the same result. * Use `✓` and `𐄂` when outputting the test's output. * Only render the summary if > 1 target. * Don't output "Tests failed". The summary, return code, and individual results already express this. * Always write to stderr. See #9710 (comment) for a justification.   Once we have streaming workunits, we will replace the log statements with immediately writing the results to the console. [ci skip-jvm-tests]
…k`, and `test` (#14303) Before: ``` ❯ ./pants --black-skip fmt src/python/pants/util/strutil.py 07:22:21.51 [INFO] Completed: Format with Autoflake - autoflake made no changes. 07:22:22.52 [INFO] Completed: Format with docformatter - Docformatter made no changes. 07:22:22.54 [INFO] Completed: Format with isort - isort made no changes. - Black skipped. ✓ Docformatter made no changes. ✓ autoflake made no changes. ✓ isort made no changes. ``` After: ``` ❯ ./pants --black-skip fmt src/python/pants/util/strutil.py 07:22:01.24 [INFO] Completed: Format with Autoflake - autoflake made no changes. 07:22:01.41 [INFO] Completed: Format with docformatter - Docformatter made no changes. 07:22:01.70 [INFO] Completed: Format with isort - isort made no changes. ✓ Docformatter made no changes. ✓ autoflake made no changes. ✓ isort made no changes. ``` As we've added more tools, rendering skipping is somewhat noisy. More importantly, it causes a problem when tools want to disable themselves via some complex logic that requires the Rules API. For example, with Go, we don't know if a `go_package` has tests in it or not until compiling the code and running our analyzer: https://github.com/pantsbuild/pants/blob/0488605f54ef3602797d68a5c4797dea9d706b14/src/python/pants/backend/go/goals/test.py#L192-L193 With the upcoming `regex-lint` (formerly `validate`), we want to only enable the linter if `[regex-lint].config` is set up: #14102. In both these cases, it is annoying & noisy to render the skipped tools in the summary. I wasn't anticipating these use cases when designing this summary in 2020: #9710. We will still log that the tool is skipped at `-ldebug`, which helps if users are confused why something isn't showing up. [ci skip-rust] [ci skip-build-wheels]
Problem
It's confusing which linters and formatters have run. For example, docformatter will simply put the file name with no context when there is a lint failure, like this:
Goals of the design:
Result
We always write to stderr for consistency and to follow the Unix philosophy. Because the output mangles multiple tools into one stream, the output cannot be safely piped to other tools. (Instead, we should add support for writing tool output to specific files, like we allow with Pytest.)
[ci skip-rust-tests]
[ci skip-jvm-tests]