https://pantsbuild.org/ logo
#development
Title
# development
b

bitter-ability-32190

10/11/2022, 8:19 PM
Working on adding
fix
, Q on some UX. Should
fix
include formatters by default (and have a
--skip-formatters
flag, akin to
lint
)? I think it's a nicer UX as
fix
isn't meant to be run in a loop (buyer beware) as it'll blast your code. Running both fixers/formatters means people can still have a one-command "fix everything" goal.
fmt
will remain unchanged.
w

wide-midnight-78598

10/11/2022, 8:21 PM
Wasn't there already a whole discussion about this - with a landed conclusion? Possibly in Slack, possibly in google docs?
b

bitter-ability-32190

10/11/2022, 8:24 PM
This is different. The result of the previous discussion was adding
fix
. This is on top of that. Although does make me think, gonna update the Q
updated
w

wide-midnight-78598

10/11/2022, 8:27 PM
Ooof, yeah, that loop thing - didn't think about that
h

hundreds-father-404

10/11/2022, 8:32 PM
Hm, definitely useful for
fix
to also run
formatters
as toggled by an option. The thing I'm not sure about is the default - I bias towards defaulting to it not running
fmt
because it does more work than the user wants, and it's two ways of doing the same thing
w

wide-midnight-78598

10/11/2022, 8:34 PM
Seems silly to have
./pants fix --also-fmt
when you can write
./pants fix fmt
👍 1
Even if it's persisted in toml
I thought the "goal" (no pun) was to merge the two, rather than add a new
fix
goal?
h

hundreds-father-404

10/11/2022, 8:36 PM
I thought the "goal" (no pun) was to merge the two, rather than add a new fix goal?
the conclusion was it's better to have them separate
w

wide-midnight-78598

10/11/2022, 8:37 PM
ahhhhh, ok ok ok, didnt recall that part. Thought it was a deprecation plan for one with a default alias
b

bitter-ability-32190

10/11/2022, 8:44 PM
The utility of all in one is that they get run in parallel, and it's less goals to remember if you don't care about the difference
h

hundreds-father-404

10/11/2022, 8:45 PM
I bias towards it being only fix by default, but you can set an option to also run fmt. And then orgs can set that in pants.toml so their devs don't have to think about it
b

bitter-ability-32190

10/11/2022, 8:45 PM
Works for me!
h

hundreds-father-404

10/11/2022, 8:45 PM
but, presumably they'd still be teaching their devs about
fmt
when they want to avoid
fix
? So, I don't think this is applicable
and it's less goals to remember if you don't care about the difference
You'd have to teach:
fmt if you don't want fixers, otherwise just use fix!
which doesn't seem much better than
fmt
if you don't want fixers, otherwise
fmt fix
b

bitter-ability-32190

10/11/2022, 8:56 PM
I still think there's value in less mental space and actual performance implications of running them in parallel
h

hundreds-father-404

10/11/2022, 8:57 PM
maybe have it be a dedicated follow up PR?
b

bitter-ability-32190

10/11/2022, 9:05 PM
Welllllllllllll it also lends it itself easily in implementation, because we can make fmt types inherit from fix types 🙈
Also I just plan on teach "just use fix" since it's simple and my engs like that
Anyone else wanna weigh in? @happy-kitchen-89482 @witty-crayon-22786 @fast-nail-55400 I'm going to start implementing the relationship between
fmt
and
fix
code soon 🙂
w

witty-crayon-22786

10/12/2022, 8:16 PM
regarding performance: https://github.com/pantsbuild/pants/issues/10542 would help, but primarily in cases where the previous goal didn’t make a change
b

bitter-ability-32190

10/12/2022, 8:18 PM
I should've been clearer too. Even if we had that issue solved (which wouldn't help this because we cant run
fmt
and
fix
in parallel) there's additional perf for constructing-and-running the batches all in
fix
. Instead of batching-and-running in fmt + batching-and-running in fix
w

witty-crayon-22786

10/12/2022, 8:20 PM
which wouldn’t help this because we cant run
fmt
and
fix
in parallel
we can: see the discussion on the ticket i linked.
b

bitter-ability-32190

10/12/2022, 8:20 PM
Both are goals with sideffects though, so running them in parallel seems suuuuper dangerous 😮
w

witty-crayon-22786

10/12/2022, 8:20 PM
they would run up to their critical section (the write to the Workspace) in parallel, but then the second one on the CLI would wait.
yes: that is addressed on the ticket.
b

bitter-ability-32190

10/12/2022, 8:21 PM
That seems late, no? first-one-wins vs. run serial
but would still be restarted to observe sideeffects caused by other goals.
ah
w

witty-crayon-22786

10/12/2022, 8:21 PM
b

bitter-ability-32190

10/12/2022, 8:21 PM
So possible, but still liekly worse perf. Got it
👍 1
w

witty-crayon-22786

10/12/2022, 8:23 PM
So possible, but still liekly worse perf. Got it
yea:
but primarily in cases where the previous goal didn’t make a change
how much would be reusable would depend on the edits involved
but since
fmt
and
fix
are each internally sequential, the actual performance impact here is not as great as it would be for
lint
UX wise, it’s slightly annoying to have new things to need to type, so possibly dependent on which things are actually moving into `fix`… would it be things that i would want to run inside my edit-check/test loop?
b

bitter-ability-32190

10/12/2022, 8:27 PM
isort
and
autoflake
being in different goals makes me think it's relatively likely for a refactor. Not to mention first-one-wins could yield some really annoying have-to-rerun scenarios. It may be
isort
"wins",
autoflake
reruns and changes it such that
isort
would then fail if rerun 😞
I'm increasingly convinced
fix
should encompass
fmt
and the default of when is up for debate
w

witty-crayon-22786

10/12/2022, 8:28 PM
It may be
isort
“wins”,
autoflake
reruns and changes it such that
isort
would then fail if rerun
not that i necessarily disagree* with you, but this could already be the case, even within a single goal.
b

bitter-ability-32190

10/12/2022, 8:30 PM
Yes but the order in
backend_backaged
is respected, and if the user crafted those purposefully in a way where "it works" they're now semi-SOL
w

witty-crayon-22786

10/12/2022, 8:32 PM
in that case, it sounds more like the issue is that there would be a “wrong” order to run
fix
and
fmt
in
which is semi-obvious perhaps, but having
fix
always run formatters would rule it out, so.
b

bitter-ability-32190

10/12/2022, 8:33 PM
It all depends on plugin order I think. There's no winning if the user has:
Copy code
fixer1, formatter1, fixer2, formatter2
Unless it's in one dedicated goal running them serially
Although mayyyyyybe "fixers" first could win. i could see that argument
Ok slightly less convinced. Fixers should never need to rerun due to formatting changes
I'm settling on doing this, the default I struggle with
Here's my thoughts: "Why have both in
fix
?" • Having one goal do both is nicer UX for those who don't care about the difference (I actually suspect it's a majority of Pants users. The difference really only shines for
--loop
). • There are tangible increased performance gains, even over the "multiple goals in parallel" feature request • It is thematically correct, as formatters fix style So then it comes down to the default: "Default = run both" • The perfect UX for the "I don't care about the difference" crowd. The "I do care" crowd also already is likely fiddling with options anyways (
--loop
). • Maintains the thematic correctness without options twiddling • If we make concessions, we can purposefully run fixers first, which means doing this is "always right" (as opposed to reminding people to run
fix fmt
instead of
fmt fix
) "Default = only run fix" • I think a subset of people would be surprised this new goal does old goal stuff. • Also slight confusion on whether to enable to option, or run
fix fmt
Given that, my plan forward is: Run formatters in fix Make that the default as well, disabled by an option Then fixing all your code is ./pants fix :: , users who want the difference can still do ./pants --loop fmt, then run fix when they want. (Like lint, it is likely that the format process results are cached)
I plan on fleshing out the help string for these goals to be more descriptive: https://github.com/pantsbuild/pants/issues/16632