https://pantsbuild.org/ logo
Join Slack
Powered by
Working on adding `fix`, Q on some UX. Should `fi...
# development
b
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
Wasn't there already a whole discussion about this - with a landed conclusion? Possibly in Slack, possibly in google docs?
b
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
Ooof, yeah, that loop thing - didn't think about that
h
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
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
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
ahhhhh, ok ok ok, didnt recall that part. Thought it was a deprecation plan for one with a default alias
b
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
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
Works for me!
h
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
I still think there's value in less mental space and actual performance implications of running them in parallel
h
maybe have it be a dedicated follow up PR?
b
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
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
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
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
Both are goals with sideffects though, so running them in parallel seems suuuuper dangerous 😮
w
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
That seems late, no? first-one-wins vs. run serial
but would still be restarted to observe sideeffects caused by other goals.
ah
w
b
So possible, but still liekly worse perf. Got it
👍 1
w
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
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
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
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
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
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
Open in Slack
PreviousNext
Powered by