Allow using `--import` option with Autoflake #16192

tdimiduk · 2022-07-15T18:46:05Z

The linter is currently hardcoded to do "--remove-all-unused-imports", which
unfortunately does not work for all repos since imports can have side effects.
With this removed a user can still provide the argument if they desire it, but
it is possible to run without it.

tdimiduk · 2022-07-15T18:57:01Z

Closing because it would break people who rely on this behavior.

Track this issue instead: #16193

Move "--remove-all-unused-imports" to a default argument rather than a hardcoded portion of the command for autoflake. This allows a user to override if the'd prefer a more conservative run of autoflake.

tdimiduk · 2022-07-15T19:55:05Z

#16193

tdimiduk · 2022-07-15T21:20:50Z

As a local test of this, I edited my pants.toml to contain

[autoflake]
args = []

and ran ./pants -ldebug fmt ::

We can see here that --remove-all-unused-imports is not present.

[DEBUG] spawned local process as Some(67697) for Process { argv: ["./autoflake.pex_pex_shim.sh", "--in-place", "src/python/pants/backend/helm/util_rules/sources_test.py", ...

where without the edit to pants.toml

[DEBUG] spawned local process as Some(68035) for Process { argv: ["./autoflake.pex_pex_shim.sh", "--in-place", "--remove-all-unused-imports", "src/python/pants/backend/scala/subsystems/scalatest.py",

--remove-all-unused-imports does appear as before

benjyw

Nice! Thanks for this improvement.

benjyw · 2022-07-16T20:47:54Z

src/python/pants/option/option_types.py

@@ -789,6 +789,7 @@ def __new__(
        # This should be set when callers can alternatively use "--" followed by the arguments,
        # instead of having to provide "--[scope]-args='--arg1 --arg2'".
        passthrough: bool | None = None,
+        default: list[str] | None = None,


The mypy error in CI leads me to believe that the type annotation here should be _MaybeDynamicT[list[_ListMemberT]]. But @Eric-Arellano is more informed than I am about the types here.

@thejcannon thoughts? I think the issue is the | None? We should be able to not use _MaybeDynamicT because this type hint can be a subset of the original union.

We'd want it to be maybe dynamic though. I agree with Benjy, although type hinting is hard so it might still 💥

Eric I don't quite understand your comment

In the meantime, is there a workaround to unblock landing this?

Is it blocked? I think this just needs to be changed to:

Suggested change

default: list[str] | None = None,

default: _MaybeDynamicT[list[str]] = [],

FWIW we do it above...

https://github.com/tdimiduk/pants/blob/b47224962aea8fc442c94bd3305242c82011b585/src/python/pants/option/option_types.py#L641
https://github.com/tdimiduk/pants/blob/b47224962aea8fc442c94bd3305242c82011b585/src/python/pants/option/option_types.py#L722

and most importantly: https://github.com/tdimiduk/pants/blob/b47224962aea8fc442c94bd3305242c82011b585/src/python/pants/option/option_types.py#L219

I agree it's dangerous. But we're never mutating the value (its just the default) so personally 🤷‍♂️

Okay - this PR isn't the hill for me to die on then. We really appreciate your contribution @tdimiduk and it isn't your responsibility to fix something that was already not ideal

So, I think the best thing would be @tdimiduk to change the list[str] | None = None type hint to be list[str] = []. Then, run ./pants --changed-since=HEAD check and double check you're good to go.

Wouldn't the right thing to do be to use the suggestion above? Is there a reason we'd want to disallow dynamic values?

Ah true. I merged the commit. Thanks

Sorry for the delay in getting back to this. I got sick while traveling and am just catching up with things now.

I don't love the mutable [] as a default (and was what I clanked on finding a better solution to before I got sick). If you are happy with this (as it being not worse than is done elsewhere), I'm happy.

I'm seeing that suggestion applied, and so this should be ready to go with another run of the CI? Is there anything else I should do?

Co-authored-by: Joshua Cannon <joshdcannon@gmail.com>

src/python/pants/option/option_types.py

Eric-Arellano

Thanks again!

Eric-Arellano · 2022-07-21T21:48:34Z

src/python/pants/option/option_types.py

@@ -802,6 +803,7 @@ def __new__(
                    """
                )
            ),
+            default=default,


Lol what. The new error message makes no sense to me. The types are the same according to the error. I assumed _ListMemberT was the same name, but not the same value -- but that doesn't seem to be it.

src/python/pants/option/option_types.py: note: In member "__new__" of class "ArgsListOption": [276](https://github.com/pantsbuild/pants/runs/7458162823?check_suite_focus=true#step:9:277) src/python/pants/option/option_types.py:806:21: error: Argument "default" to [277](https://github.com/pantsbuild/pants/runs/7458162823?check_suite_focus=true#step:9:278) "__new__" of "_ListOptionBase" has incompatible type [278](https://github.com/pantsbuild/pants/runs/7458162823?check_suite_focus=true#step:9:279) "Union[Callable[[Any], Any], List[_ListMemberT]]"; expected [279](https://github.com/pantsbuild/pants/runs/7458162823?check_suite_focus=true#step:9:280) "Union[Callable[[Any], Any], List[_ListMemberT]]" [arg-type] [280](https://github.com/pantsbuild/pants/runs/7458162823?check_suite_focus=true#step:9:281) default=default, [281](https://github.com/pantsbuild/pants/runs/7458162823?check_suite_focus=true#step:9:282) ^

At what point should we ignore the error?

File an issue to mypy?

We can type ignore and reference it here

Hopefully callsites are still type checked correctly

I've gone with the type ignore for now. @Eric-Arellano or @thejcannon do you have more context for the mypy bug and want to file it? I can do it, but I'd need to spend some time figuring out what is going on here to file a useful bug.

Weird right? This goes beyond my understanding of type annotations. I think a type ignore annotation is the way to go.

I would expect mypy devs to want us to use main to reproduce, and bumping mypy in the repo isn't fun. Let's just leave it for now and really hope that its fixed in future versions and we get that "unnecessary type ignore" warning.

tdimiduk · 2022-07-21T22:31:33Z

src/python/pants/option/option_types.py

@@ -216,7 +216,7 @@ def __new__(
        cls,
        flag_name: str | None = None,
        *,
-        default: _MaybeDynamicT[list[_ListMemberT]] = [],


Discovered there is already a check on line 234 to put in [] if default is falsy. Seems like making the default here Optional and passing in None is preferable to using the mutable default argument. Happy to back it out if we don't want to change the signature here.

Agreed, since we can't get the type check to work anyway, better to not do the bad thing with the mutable default for no purpose.

tdimiduk · 2022-07-21T22:31:58Z

src/python/pants/option/option_types.py

@@ -802,6 +803,7 @@ def __new__(
                    """
                )
            ),
+            default=default, # type: ignore[arg-type]


Went with the type ignore for now since it does seem like this might be a mypy bug.

Thanks for your patience with all this! Mypy is a bit of a challenge sometimes.

You are welcome. I'm glad to have typing in python, just eagery looking forward to it becoming better typing over time.

Eric-Arellano · 2022-07-22T19:49:59Z

Looks like a Black failure. I pushed the fix.

tdimiduk · 2022-07-22T20:43:06Z

Looks like a Black failure. I pushed the fix.

Thanks.

benjyw · 2022-07-22T21:41:36Z

Thanks again for the fix!

The linter is currently hardcoded to do "--remove-all-unused-imports", which unfortunately does not work for all repos since imports can have side effects. With this removed a user can still provide the argument if they desire it, but it is possible to run without it. [ci skip-build-wheels] [ci skip-rust] # Conflicts: # src/python/pants/option/option_types.py

…16275) The linter is currently hardcoded to do "--remove-all-unused-imports", which unfortunately does not work for all repos since imports can have side effects. With this removed a user can still provide the argument if they desire it, but it is possible to run without it. [ci skip-build-wheels] [ci skip-rust] Co-authored-by: Tom Dimiduk <tom@dimiduk.net>

The linter is currently hardcoded to do "--remove-all-unused-imports", which unfortunately does not work for all repos since imports can have side effects. With this removed a user can still provide the argument if they desire it, but it is possible to run without it. [ci skip-build-wheels] [ci skip-rust]

tdimiduk closed this Jul 15, 2022

Remove hardcoded argument from autoflake linter

f9f10b7

Move "--remove-all-unused-imports" to a default argument rather than a hardcoded portion of the command for autoflake. This allows a user to override if the'd prefer a more conservative run of autoflake.

tdimiduk reopened this Jul 15, 2022

tdimiduk force-pushed the main branch from b788abd to f9f10b7 Compare July 15, 2022 19:53

Thomas Dimiduk added 3 commits July 15, 2022 16:50

pass default through super call

d7ac781

add comment about why default was added

69ee9af

add issue to comment

b472249

Eric-Arellano approved these changes Jul 15, 2022

View reviewed changes

Eric-Arellano added category:new feature category:bugfix Bug fixes for released features and removed category:new feature labels Jul 15, 2022

Eric-Arellano changed the title ~~Remove hardcoded argument from autoflake linter~~ Allow using --import option with Autoflake Jul 15, 2022

Eric-Arellano added category:new feature category:bugfix Bug fixes for released features and removed category:bugfix Bug fixes for released features category:new feature labels Jul 15, 2022

benjyw approved these changes Jul 16, 2022

View reviewed changes

benjyw reviewed Jul 16, 2022

View reviewed changes

Update src/python/pants/option/option_types.py

3d10242

Co-authored-by: Joshua Cannon <joshdcannon@gmail.com>

benjyw reviewed Jul 21, 2022

View reviewed changes

src/python/pants/option/option_types.py Outdated Show resolved Hide resolved

Another attempt to fix the type annotation

d530ae7

Eric-Arellano approved these changes Jul 21, 2022

View reviewed changes

Eric-Arellano added the needs-cherrypick label Jul 21, 2022

Eric-Arellano added this to the 2.13.x milestone Jul 21, 2022

Eric-Arellano enabled auto-merge (squash) July 21, 2022 21:10

Eric-Arellano reviewed Jul 21, 2022

View reviewed changes

add type ignore and use None instead of []

e64047a

auto-merge was automatically disabled July 21, 2022 22:29
Head branch was pushed to by a user without write access

tdimiduk commented Jul 21, 2022

View reviewed changes

stuhood enabled auto-merge (squash) July 22, 2022 17:58

Eric-Arellano added 2 commits July 22, 2022 14:21

Merge branch 'main' of github.com:pantsbuild/pants into tdimiduk/main

8f65014

Fix fmt

6828a98

stuhood merged commit 14e0122 into pantsbuild:main Jul 22, 2022

stuhood removed the needs-cherrypick label Aug 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow using `--import` option with Autoflake #16192

Allow using `--import` option with Autoflake #16192

tdimiduk commented Jul 15, 2022

tdimiduk commented Jul 15, 2022

tdimiduk commented Jul 15, 2022

tdimiduk commented Jul 15, 2022

benjyw left a comment

benjyw Jul 16, 2022

Eric-Arellano Jul 16, 2022

thejcannon Jul 17, 2022

stuhood Jul 18, 2022

thejcannon Jul 19, 2022

thejcannon Jul 19, 2022

Eric-Arellano Jul 19, 2022 •

edited

Loading

thejcannon Jul 19, 2022

Eric-Arellano Jul 19, 2022

tdimiduk Jul 21, 2022

Eric-Arellano left a comment

Eric-Arellano Jul 21, 2022

thejcannon Jul 21, 2022

tdimiduk Jul 21, 2022

benjyw Jul 22, 2022

thejcannon Jul 22, 2022

tdimiduk Jul 21, 2022

benjyw Jul 22, 2022

tdimiduk Jul 21, 2022

benjyw Jul 22, 2022

tdimiduk Jul 22, 2022

Eric-Arellano commented Jul 22, 2022

tdimiduk commented Jul 22, 2022

benjyw commented Jul 22, 2022

	default: list[str] \| None = None,
	default: _MaybeDynamicT[list[str]] = [],

Allow using --import option with Autoflake #16192

Allow using --import option with Autoflake #16192

Conversation

tdimiduk commented Jul 15, 2022

tdimiduk commented Jul 15, 2022

tdimiduk commented Jul 15, 2022

tdimiduk commented Jul 15, 2022

benjyw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano commented Jul 22, 2022

tdimiduk commented Jul 22, 2022

benjyw commented Jul 22, 2022

Allow using `--import` option with Autoflake #16192

Allow using `--import` option with Autoflake #16192

Eric-Arellano Jul 19, 2022 •

edited

Loading