https://pantsbuild.org/ logo
Join Slack
Powered by
Next question: I'd like to run autoflake, but pant...
# general
f
Next question: I'd like to run autoflake, but pants seems to default to running it with a 
--remove-all
flag that I'm not ready for. I've tried passing my own 
args
but that doesn't seem to replace it. How do I find out and/or change what command pants is actually running?
f
You should see the command being run in debug output by passing 
-ldebug
option.
w
./pants fmt -ldebug ::
Gahhh, beaten...
f
Looks like it's actually
Copy code
./pants -ldebug fmt  ::
but thanks for pointing me at the flag
w
Search for something like: 
/autoflake.pex_pex_shim.sh
f
In the pants repo?
w
Sorry, in the debug output - the Process with thatshould be roughly where the args are specified
f
pants does seem to be running with 
"--remove-all-unused-imports"
which seems like it might be a bad default. Is there a way to suppress that?
w
What happens when you modify the args in 
pants.toml
?
f
i have already, I'm running with
Copy code
args = "['--imports=pandas,numpy']"
which causes it to run with
Copy code
"./autoflake.pex_pex_shim.sh", "--in-place", "--remove-all-unused-imports", "--imports=pandas,numpy", ...
I'm fine with the 
--in-place
but I think we have some imports with side effects, so I'd like to start with something safer
w
Oh, it's appending... Hmm, just a sec, let me check the code
w
can also set the 
skip_autoflake=True
flag on targets that you’re not ready for.
f
I'd like to autoflake everything (to get rid of unused stdlib imports), but I don't want to remove imports other than stdlib (and a few known safe ones)
👍 1
w
f
Shall I file an issue about the 
--remove-all-unused-imports
? It seems like that might be better to let people opt in to
or I guess that's simple enough I should just do a PR if it would be welcome
💯 1
w
yea, that would be welcome!
w
Probably fine as a default, but there should at least be an opt-out I would have assumed?
➕ 1
w
maybe via a pants option which defaults to true (for now)
coke
w
I thought there was a way to specify default args on a Subsystem, which are overridable
As in, I thought there was the "append" option and the "overwrite" option
w
there is, yea. could do it that way probably.
w
In lieu of flags on flags, anyways
f
w
So, as-is, it probably shouldn't be accepted - since it's a breaking change to everyone who uses autoflake right now. Ideally something more like, a default set of args matching today, which can be overwritten might be a safer, backwards compatible approach
f
Understood. I'll file a issue for now
👍 1
h
yeah, I think that we should have 
[autoflake].args
default to 
["--remote-all-args"]
. Then you can append to it, replace it, etc
f
yeah, that would be a better option
h
@freezing-vegetable-92896 are you interested in submitting a PR? It's I think only a few lines and I can write up some instructions 🙂
f
I'm happy to submit a PR, especially if you point me at where the defaults are defined
❤️ 1
it'd take me a bit of tracing to find it by myself
h
commented 🙂
f
Updated my PR to do what you suggested: https://github.com/pantsbuild/pants/pull/16192
h
sweet! you're missing updating 
super().__new__()
also would be great to try this out locally to make sure it works. In pantsbuild/pants, do something like override 
[autoflake].args
w
I think the tests also need to be updated? https://github.com/pantsbuild/pants/blob/main/src/python/pants/backend/python/lint/autoflake/rules_integration_test.py I think it checks for auto-removing unused imports
Sorry, but we should a) confirm that works, and b) confirm it can be overridden
f
Updated with the super call and a comment about a local test
Looking at the tests @wide-midnight-78598 pointed me at now, though I'm going to have to step out for dinner soon and may have to come back to this
h
Re tests: 1) the existing tests should continue to work w/o modification. Otherwise, your PR is a breaking API change (like your first draft) 2) I debated asking you to add a dedicated test for being able to override the defaults, and am thinking it's maybe not worth the extra test. Every test has a cost, including slower CI and more code. So it's a tradeoff if it's worth it. Maybeee it's worth it, or we should modify an existing test, but this seems pretty trivial and our options system already has good tests for 
default
f
I was able to run the existing test and confirm that it passes locally with the second draft, so hopefully that means no API change
❤️ 2
What is the consensus of whether this needs a dedicated test or modification of an existing test?
h
I personally vote no, this seems pretty unlikely we're going to regress and break it. If you wanted to reduce that risk, you could add a comment like this above the line that sets `default`:
See https://github.com/pantsbuild/pants/issues/16193 for why we set this as the default.
f
Added a comment
It looks like the PR needs a label assigned, but maybe someone with write access to the repo has to assign the label? Or am I looking in the wrong place?
h
Ah yeah, added. Not sure whether to classify as new feature or bug fix -- I'll go with new feature since we're giving you a new capability we didn't have before. (This is used to generate the changelog)
f
Sure. I filed my issue as a bug since it prevented me from doing something it seemed like I should be able to do, but new feature works too
👍 1
h
yeah fair, okay switched back lol
👍 1
h
Yeah, I think we don't need a dedicated test here, since we're relying on extremely well-tested behavior of the options system. Thanks for the patch @freezing-vegetable-92896!
➕ 1
I commented with a possible fix for the mypy error.
w
That's true, if the existing tests still pass - I suppose that would be test enough (ie. testing that existing behaviour isn't broken)
➕ 1
5 Views
Open in Slack
PreviousNext
Powered by