https://pantsbuild.org/ logo
Join Slack
Powered by
Why is it then that I see > 1000 lines of black...
# general
f
Why is it then that I see > 1000 lines of black linter calls even when I don’t touch python? Do pull requests use a different/individual cache?
h
Because the CI job runs 
build-support/bin/ci.py --lint
, which runs 
./pants --tag='-nolint' lint --per-target-caching ::
The effect is running Black once for every single target 😕 that’s what 
--per-target-caching
results in
f
so, 
<http://ci.py|ci.py> --lint
doesn’t use a cache in that case?
h
Well, CI uses a remote cache. The remote cache will get the prior result of running Black on target 1 without having to do any new work. But, it will still output the result of running on target 1 The remote cache helps with the speed of that shard; it does not help with the verbosity of output
f
ah
I see
h
It’s not ideal. We could in CI turn off 
--per-target-caching
, but that results in the shard taking about 10 minutes longer on average iirc because we’re running on every single target The better solution is the upcoming project to remove 
--per-target-caching
into a merged solution that gets the benefits of both
f
We could pass 
--quiet
to black in CI
which would cause it to suppress output for non-failures
h
Agreed. Feel free to put up that PR if you have a moment and I’ll approve 🙂 you’d want to modify 
pants.travis-ci.toml
f
w
i don't think it makes sense to show output on success for these linters, i don't think. yea, black's output is cute, but we're up to 4 linters, and it's mostly just noise, regardless of 1-run-per-target or 1-run-for-all-targets.
f
yeah
I feel like that’s something that ought to be handled at the lint console rule level
do console rules have common options around output?
h
i don’t think it makes sense to show output on success for these linters, i don’t think.
My main hesitation is that Pants is now introspecting the tools and modifying the results, rather than us simply wrapping the tools. That’s an important philosophical distinction
w
it doesn't need to modify it in any way. just not render it on success.
f
yeah, that
h
do console rules have common options around output?
No, they don’t, beyond things like 
--sep
and 
--output-file
iirc. Also now renamed to goal rules
it doesn’t need to modify it any way. just not render it on success.
Sure, tactically, we can inspect the return code and not render if successful. To do that, we need to either in 
black/rules.py
mutate the FmtResult so that the 
stdout
attribute is empty, or we need to generalize this filtering in 
<http://fmt.py|fmt.py>
. Also, when using 
--no-per-target-caching
, I personally quite like Black saying “ran over 71 files.” It’s helpful when I’m doing something like 
./v2 --changed-since=master fmt
to know how many files I’ve changed
w
i think that this is general, for all 
lint
output.
we could perhaps differentiate "success" from "warn" and "fail" at the return-code level, and then render warn/fail.
h
This gets to Pierre’s blog post. Every tool handles return codes differently and things like warnings differently. Maybe, we do end up trying to create the unified interface he mentioned (and I think implemented at Twitter). But, my point is that by us deciding to no longer render Black’s stdout in some cases, we are no longer “a simple wrapper around underlying linters and formatters”. We are now in the business of interpreting the results. For example. Black has a 
--verbose
option. Now we are removing the ability for the user to run with 
--verbose
on successful cases, which they might want to do to check which precise files Black ran over.
w
it's less about individual tools, and more about general consistency. what we have done for things like 
--verbose
in many other cases is propagate pants' own log level to tools that are spawned.
because otherwise you need to know too many permutations of per-tool flags to make progress.
and i don't think the goal is to be a "simple wrapper"... it was in Pierre's case. simple, yes. but consistent rather than thinly wrapped.
h
Right, we could do that. My point is that we need to make a philosophical decision whether Pants continues to simply wrap formatters and linters or whether it has this unified interface we create. In other words, “not showing Black’s output when successful” is less innocuous than it sounds. It means that we’ve decided we do want to get closer to Pants now having this unified interface
w
the alternative is for 10 linters to output ten different formats. very quickly it becomes unreadable noise.
h
and i don’t think the goal is to be a “simple wrapper”
To clarify, I argue that right now we are simple wrappers. We don’t put any restrictions on the passthru args you can give and we also allow you to specify a config file with whatever options you want On the extreme opposite, we don’t allow config files or passthrough args. Everything is a Pants option
w
i think that's a false dichotomy. i'm not suggesting adjusting which options are legal. i'm suggesting that pants should attempt to render "the most useful information" by default.
yes, that's subjective.
h
i think that’s a false dichotomy. i’m not suggesting adjusting which options are legal.
We arguably would need some level of validating args, though, if we do this. It likely no longer makes sense to allow users to specify 
--black-args='--verbose'
and 
--black-args='--quiet'
if Pants is already doing its own thing with output. I’m not saying we should absolutely avoid this, only that I think we need to be really careful to open the door.
w
h
Parametrizing this would be much better, agreed
And it would be great to add this option to V2 test. That pytest output is so ridiculously verbose in CI
w
yes.
and then consistency comes back in. if possible, consistency across the goals would be good (without overoptimizing for it.)
👍 1
h
Cool, parametrizing sounds good to me. Although, favor to please wait one day before anyone puts up PRs. (I’m about to put up the Target API rewrite of 
fmt
and 
lint
which I’ve been working on for 3 days and want to avoid merge conflicts)
w
in the sense that a tool would classify something as warn, if it knew how.
and then at the top level parametization would decide what to do with process outputs.
...very analogous to log levels, i suppose.
h
Probably more complex than only looking at return code. E.g. Pytest will use a return code of 0 but print deprecations to stderr. There, we’d inspect stderr to determine if there are warnings on success cases For Black, it always prints to stderr no matter what. No idea how to handle warnings there outside of regex matches on the stderr attribute, which is icky
w
yep
well. yes. it's very tool specific.
h
At the very least we need to add banners demarcating which output came from which tool.
👍 1
f
yeah
h
Right now it's just a direct concatenation of the output from each tool, which I find fairly confusing
👍 1
f
I think that would require tagging the results with the tool, which right now they are not
h
That should be simple to do. We already do it with Test for example. We would add an attribute to 
FmtResult
and 
LintResult
for the name of the tool, e.g. 
"black"
vs. 
"flake8"
. Then, we could add that to the output as a banner
f
that and collating by tool would make for slick output
w
... mm. This needs another round of design in general. Because we're currently not rendering output until the end either. And that's questionable for both tests and lint.
f
yeah
h
Because we’re currently not rendering output until the end either.
I’d love to change that, but how is that possible? We’re using 
await MultiGet
and we can’t do anything with the results until it’s all done. I suppose the 
coordinator
rule pattern?
w
Logging is the current way. But if it isn't enough, we'd want to consider adding more native facilities
5 Views
Open in Slack
PreviousNext
Powered by