https://pantsbuild.org/ logo
Join Slack
Powered by
Hey guys, is there any specific reason why FmtRequ...
# plugins
s
Hey guys, is there any specific reason why FmtRequest and FixRequest extend LintRequest and inherit it's rules? My formatter appears in 
pants lint
output: https://github.com/pantsbuild/pants/pull/20358#pullrequestreview-1811878481 I was able to fix it, but the fix is kinda hacky: https://github.com/pantsbuild/pants/pull/20358/commits/ade03f80272895533d7ab5fae032b13b5726522b
b
That's precisely why. So they run in lint (and therefore your CI can run 
lint ::
and it checks all your linters and fixers and formatters). Otherwise there's no easy way to have CI check if the files are formatted
s
but I have a separate lint request, I don't need format to be lint at the same time
b
You'll have to roll your own then and not use the rules that are provided. Can I ask why though? The reason is structured like this is so that fmt and lint reuse the exact same process so you get much better caching
s
black uses different commands for that:
Copy code
black format
black format --check # lint
black fix 
black fix --check # lint
b
Oh black got a fix command? Neato
s
ops, sorry, I was talking about ruff
ruff got format command šŸ™‚
b
There's a PR to get ruff working upstream, so if you want you can wait, or copy it locally šŸ˜‰
I imagine it might still re-use the fmt/fix process for lint
s
I'm the one who is fixing it
b
šŸ‘€
s
take a look at the pr ^
b
Ah well, yeah that's the reasoning
s
b
Running a new process with 
--check
is a waste of energy since the result is "yes" or "no", and if the answer is "no" the solution is to rerun fmt or fix anyways
And if the answer is yes, it would've had the same result as just running the tool on the code
s
What is the proper way to do it? I would appreciate if you could write in the pr your comments
b
I'm not sure I understand the question. If you inherit from the Fmt/Fix classes and provide the rule(s) for running the processes you should be set
s
I noticed 
pants lint
now runs 4 checkers instead of 2:
```āœ“ ruff check succeeded.
āœ“ ruff check --fix succeeded.
āœ• ruff format failed.
āœ• ruff format --check failed.```
This is not what I want to see in the run, I only need to run 
ruff format --check
and 
ruff check
b
I think you'd just have 
ruff format
be a fmt/fix rule, and 
ruff check
be a lint rule, no?
s
b
I think I'm still lost šŸ˜•
Why can't you have one implementation dedicated to 
ruff format
and one for 
ruff check
?
As if they're two completely separate tools. 
ruff format
could be thought of as 
black
and 
ruff check
be thought of as flake8
s
Yes, that's what I'm trying to do. Right now in main branch they are entangled together, and I'm trying to make them different tools
I don't understand how pants knows how to run 
lint
if I only declare fmt rule
b
That's the inheritance. The fmt rules are re-use in 
lint
. Except when running lint, instead of modifying files in the build root, we just compare before/after and fail if they're different (and list the files which changed)
s
I've declared 4 rules: lint 
ruff check
fix 
ruff check --fix
lint 
ruff format --check
fmt 
ruff format
And when I run 
pants lint
I expect to see 2 of them, but I see 4.
b
So, let's start with 
format --check
. You don't need that since 
format
is already run in 
lint
and that inherently checks the formatting. Right?
s
I don't know for sure, but that's probably right
You mean I don't need my own lint rule 
ruff format --check
and pants will automatically add lint rule that checks 
ruff format
haven't done any changes?
šŸ‘ 1
b
Great! So then what's the difference between `check`and 
check --fix
s
check
will run all ruff lint rules, 
check --fix
will fix everything it can fix
So 
check
might show errors that are not fixable by 
check --fix
b
Is there any way to have 
check
only check for issues that aren't fixable?
s
no, I don't think so
b
Well that's a bummer šŸ˜”
So either way you'll have wasted work. I suggest maybe we adjust the rules methods for fmt/fix to allow some kind of "I know what I'm doing, please don't add the lint rules". Then you'd have: ā€¢ 
ruff format
as a format rule ā€¢ 
ruff check --fix
as a fix rule, but opting out of the 
lint
goal ā€¢ 
ruff check
as a 
lint
rule
And maybe file a feature request to 
ruff
to have an easy toggle for "check anything not fixable"
s
I already have "opt-out" implemented here https://github.com/pantsbuild/pants/pull/20358/commits/ade03f80272895533d7ab5fae032b13b5726522b Is this ok to merge as is (after I remove the format lint) or do you want me to fix it properly in the base class?
b
I meant "opt-out" at the base class level
s
b
Yeah! I'll have a comment I think tonight when I'm on my tower
Really, just wanna make sure the reader understands they they almost never want to make this false, and explain why
s
ok, I've updated the comment
b
Yeah sorry I didn't get to it last night šŸ˜ž
s
Yeah, no worries, check it out when you have some time
g
Hmm, just catching up on this convo. I'm wondering if this is the same thing I thought about between 
cargo clippy
vs 
cargo clippy --fix
... So I think this solves that use-case too. For some reason 
cargo clippy --fix
is terribly slow (and barely works!) in large codebases and so I opted out of even supporting it. I'm not sure if the same is ever true for formatting, in any tool, so opting out there seems weird (which you've already concluded, upon closer reading!).
b
Makes sense. A bummer that we sacrifice simplicity, but that's life
s
Usually fixers are not able to fix all the issues the linter shows, maybe it even makes sense to disable the option for fixers. I can imagine a situation where you setup a fixer, then you run 
pants lint
and see your tool there, you might think that it runs the linter and not the fix-compare-linter, and this might lead to missing linter messages, because you think you run the linter, but you don't. Looks like confusion potential
b
Some fixers aren't necessarily lint tools. Like autoflake
s
yes, but then you enable pyflake or flake8 or ruff and you no longer need fix-compare-linter for autoflake
most fixers have lint counterpart
b
But is the lint counterpart doing more than checking to see if the fixable issues are fixed? That's the razor
s
again, it is most probably the case. 
autoflake
, 
pyupgrade
, 
ruff
, 
cargo fix
all cannot fix all the issues the linter shows. This is a core property of any linter. If there is an issue in your code then it can be fixed either in 1 way or in many ways. If it can be only fixed in 1 way then it can be fixed by a fixer, and if it can be fixed in many ways then it can't be fixed by a fixer.
b
That's true in a general sense. I wouldn't call autoflake or pyupgrade linters though
s
I mean the counterpart linter like ruff
b
They are two different tools. With two different backends (in pants parlance) so I don't think we should conflate them together
s
most users will use autoflake or pyupgrade along with flake8 or ruff, so pants will do the same work twice
b
That's not an assumption I think we need to be making at the core level. That all fixers have a lint counterpart in another tool that will be enabled and will have feature parity
For instance, we use autoflake and disable the equivalent rules (or just never use the plugin) in flake8.
s
yeah, I understand, I'm not sure what's the best solution here. I'm just concerned that it might be confusing and might lead to missing linter errors
Maybe that's ok since it only applies to plugins authors
b
That's actually the motivation behind the inheritance. Here in the pants repo we actually had a tool that was a fixer and forgot to install the lint rule (when you had to do both)
I like the opt-out. It makes sense in certain situations, I do want the comment to be crystal clear, so I'll suggest some wording when I'm back at work.
s
šŸ‘
b
Ok sorry I'm late to the game, but put my thoughts on the PR
5 Views
Open in Slack
PreviousNext
Powered by