https://pantsbuild.org/ logo
f

fancy-queen-20734

04/14/2020, 8:06 PM
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

hundreds-father-404

04/14/2020, 8:08 PM
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

fancy-queen-20734

04/14/2020, 8:09 PM
so,
<http://ci.py|ci.py> --lint
doesn’t use a cache in that case?
h

hundreds-father-404

04/14/2020, 8:10 PM
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

fancy-queen-20734

04/14/2020, 8:11 PM
ah
I see
h

hundreds-father-404

04/14/2020, 8:12 PM
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

fancy-queen-20734

04/14/2020, 8:17 PM
We could pass
--quiet
to black in CI
which would cause it to suppress output for non-failures
h

hundreds-father-404

04/14/2020, 8:17 PM
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

fancy-queen-20734

04/14/2020, 8:38 PM
w

witty-crayon-22786

04/14/2020, 8:58 PM
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

fancy-queen-20734

04/14/2020, 8:58 PM
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

hundreds-father-404

04/14/2020, 8:59 PM
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

witty-crayon-22786

04/14/2020, 8:59 PM
it doesn't need to modify it in any way. just not render it on success.
f

fancy-queen-20734

04/14/2020, 9:00 PM
yeah, that
h

hundreds-father-404

04/14/2020, 9:00 PM
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

witty-crayon-22786

04/14/2020, 9:03 PM
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

hundreds-father-404

04/14/2020, 9:37 PM
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

witty-crayon-22786

04/14/2020, 9:39 PM
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

hundreds-father-404

04/14/2020, 9:41 PM
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

witty-crayon-22786

04/14/2020, 9:42 PM
the alternative is for 10 linters to output ten different formats. very quickly it becomes unreadable noise.
h

hundreds-father-404

04/14/2020, 9:42 PM
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

witty-crayon-22786

04/14/2020, 9:49 PM
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

hundreds-father-404

04/14/2020, 9:51 PM
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

witty-crayon-22786

04/14/2020, 9:59 PM
h

hundreds-father-404

04/14/2020, 10:00 PM
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

witty-crayon-22786

04/14/2020, 10:02 PM
yes.
and then consistency comes back in. if possible, consistency across the goals would be good (without overoptimizing for it.)
👍 1
h

hundreds-father-404

04/14/2020, 10:03 PM
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)
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

hundreds-father-404

04/14/2020, 10:05 PM
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

witty-crayon-22786

04/14/2020, 10:06 PM
yep
well. yes. it's very tool specific.
h

happy-kitchen-89482

04/14/2020, 10:44 PM
At the very least we need to add banners demarcating which output came from which tool.
👍 1
f

fancy-queen-20734

04/14/2020, 10:44 PM
yeah
h

happy-kitchen-89482

04/14/2020, 10:44 PM
Right now it's just a direct concatenation of the output from each tool, which I find fairly confusing
👍 1
f

fancy-queen-20734

04/14/2020, 10:44 PM
I think that would require tagging the results with the tool, which right now they are not
h

hundreds-father-404

04/14/2020, 10:46 PM
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

fancy-queen-20734

04/14/2020, 10:49 PM
that and collating by tool would make for slick output
w

witty-crayon-22786

04/14/2020, 10:50 PM
... 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

fancy-queen-20734

04/14/2020, 10:51 PM
yeah
h

hundreds-father-404

04/14/2020, 10:54 PM
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

witty-crayon-22786

04/14/2020, 10:56 PM
Logging is the current way. But if it isn't enough, we'd want to consider adding more native facilities
5 Views