https://pantsbuild.org/ logo
Powered by
f

freezing-vegetable-92896

07/15/2022, 6:17 PM
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

fast-nail-55400

07/15/2022, 6:19 PM
You should see the command being run in debug output by passing 
-ldebug
option.
w

wide-midnight-78598

07/15/2022, 6:20 PM
./pants fmt -ldebug ::
Gahhh, beaten...
f

freezing-vegetable-92896

07/15/2022, 6:20 PM
Looks like it's actually
Copy code
./pants -ldebug fmt  ::
but thanks for pointing me at the flag
w

wide-midnight-78598

07/15/2022, 6:21 PM
Search for something like: 
/autoflake.pex_pex_shim.sh
f

freezing-vegetable-92896

07/15/2022, 6:22 PM
In the pants repo?
w

wide-midnight-78598

07/15/2022, 6:22 PM
Sorry, in the debug output - the Process with thatshould be roughly where the args are specified
f

freezing-vegetable-92896

07/15/2022, 6:23 PM
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

wide-midnight-78598

07/15/2022, 6:24 PM
What happens when you modify the args in 
pants.toml
?
f

freezing-vegetable-92896

07/15/2022, 6:24 PM
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

wide-midnight-78598

07/15/2022, 6:27 PM
Oh, it's appending... Hmm, just a sec, let me check the code
w

witty-crayon-22786

07/15/2022, 6:28 PM
can also set the 
skip_autoflake=True
flag on targets that you’re not ready for.
f

freezing-vegetable-92896

07/15/2022, 6:30 PM
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

wide-midnight-78598

07/15/2022, 6:31 PM
f

freezing-vegetable-92896

07/15/2022, 6:32 PM
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

witty-crayon-22786

07/15/2022, 6:33 PM
yea, that would be welcome!
w

wide-midnight-78598

07/15/2022, 6:34 PM
Probably fine as a default, but there should at least be an opt-out I would have assumed?
➕ 1
w

witty-crayon-22786

07/15/2022, 6:34 PM
maybe via a pants option which defaults to true (for now)
coke
w

wide-midnight-78598

07/15/2022, 6:37 PM
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

witty-crayon-22786

07/15/2022, 6:37 PM
there is, yea. could do it that way probably.
w

wide-midnight-78598

07/15/2022, 6:39 PM
In lieu of flags on flags, anyways
f

freezing-vegetable-92896

07/15/2022, 6:46 PM
w

wide-midnight-78598

07/15/2022, 6:48 PM
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

freezing-vegetable-92896

07/15/2022, 6:52 PM
Understood. I'll file a issue for now
👍 1
h

hundreds-father-404

07/15/2022, 6:59 PM
yeah, I think that we should have 
[autoflake].args
default to 
["--remote-all-args"]
. Then you can append to it, replace it, etc
f

freezing-vegetable-92896

07/15/2022, 6:59 PM
yeah, that would be a better option
h

hundreds-father-404

07/15/2022, 6:59 PM
@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

freezing-vegetable-92896

07/15/2022, 7:00 PM
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

hundreds-father-404

07/15/2022, 7:05 PM
commented 🙂
f

freezing-vegetable-92896

07/15/2022, 7:58 PM
Updated my PR to do what you suggested: https://github.com/pantsbuild/pants/pull/16192
h

hundreds-father-404

07/15/2022, 8:01 PM
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

wide-midnight-78598

07/15/2022, 8:11 PM
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

freezing-vegetable-92896

07/15/2022, 8:50 PM
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

hundreds-father-404

07/15/2022, 8:54 PM
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

freezing-vegetable-92896

07/15/2022, 9:02 PM
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

hundreds-father-404

07/15/2022, 9:04 PM
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

freezing-vegetable-92896

07/15/2022, 9:10 PM
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

hundreds-father-404

07/15/2022, 9:12 PM
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

freezing-vegetable-92896

07/15/2022, 9:14 PM
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

hundreds-father-404

07/15/2022, 9:14 PM
yeah fair, okay switched back lol
👍 1
h

happy-kitchen-89482

07/16/2022, 8:42 PM
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

wide-midnight-78598

07/16/2022, 8:48 PM
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
Powered by