feat: add ARG001 rule #8045

ralbertazzi · 2023-05-30T18:55:20Z

As this affects the whole codebase it would be great to have a quick feedback cycle to avoid lots of painful rebases 🙏

This kind of rules doesn't obviously play well with pytest fixtures, that's why many inline ignores. On the other side, it helped detecting lots of unneeded fixtures.

tests/helpers.py

tests/conftest.py

Secrus · 2023-05-31T06:39:59Z

Honestly, I don't like this. The amount of false positives that we have to silence is just too high for my taste. I am all for doing some cleaning, but to keep this rule enabled in the long run, not really.

finswimmer · 2023-05-31T06:44:29Z

Honestly, I don't like this. The amount of false positives that we have to silence is just too high for my taste. I am all for doing some cleaning, but to keep this rule enabled in the long run, not really.

Maybe a good compromise would be to activate this rule only for the package code, but not for the tests?

Secrus · 2023-05-31T06:48:22Z

Maybe a good compromise would be to activate this rule only for the package code, but not for the tests?

For me, tests are an integral part of code, so treating them differently would be weird imho.

Disclaimer: I won't block this PR if others think it's a good change. Just sounding out my concerns.

ralbertazzi · 2023-05-31T06:50:35Z

I may have missed an obvious thing, but I didn't find a way to disable rules on entire folders on ruff 🤔
If you don't like having the rule enabled, one solution could also be to revert the enabling while still merging the improvements that it brought

radoering · 2023-05-31T14:46:02Z

Honestly, I don't like this. The amount of false positives that we have to silence is just too high for my taste. I am all for doing some cleaning, but to keep this rule enabled in the long run, not really.

That was actually my first thought as well.

one solution could also be to revert the enabling while still merging the improvements that it brought

Sounds good to me.

radoering · 2023-05-31T14:56:00Z

Maybe a good compromise would be to activate this rule only for the package code, but not for the tests?

Would also be ok for me.

I may have missed an obvious thing, but I didn't find a way to disable rules on entire folders on ruff 🤔

Did you try file patterns with *?

ralbertazzi · 2023-05-31T16:09:52Z

Silly me.. it's actually possible to disable the check on the entire tests folder. To summarize:

We keep ARG001 enabled on the main codebase
We keep ARG001 disabled on tests, while still merging the cleanup performed while keeping this rule enabled

We now have a total of 2 additional # noqa pragmas in total. Seems small enough to call it not noisy :)

dimbleby · 2023-05-31T18:01:29Z

pyproject.toml

@@ -102,6 +102,7 @@ unfixable = [
 target-version = "py38"
 line-length = 88
 extend-select = [
+    "ARG001", # flake8-unused-arguments - Rules other than ARG001 result in false positives only


I'm not seeing any false positives when I enable ARG, only true positives.

Did you report a bug to ruff for any false positives that you see?

Sorry, I might have explained myself incorrectly. They are true positives but provide meaningless action (like a #noqa pragma), so I don't see value in keeping them enabled.

Then I'd prefer - if the consensus is that checking for unused arguments is useful at all - to turn them all on and # noqa the ones that are deliberate (usually because the method must comply with some base class or protocol).

There aren't very many such examples, certainly if you're limiting yourself to src.

The point of enabling rules is not only to do a one-time clean-up of the codebase, it's to prevent future mistakes. Seems inconsistent to say "we don't want unused arguments in functions, but we're ok with them in methods, class methods, static methods, lambdas".

Also I think the warnings can mostly be suppressed by using an _ for [the first character of] the unused parameter name, which hardly clutters the code at all - makes it clearer, if anything.

This is kind of the opposite of #8045 (comment) though. Hard to find consensus 😄

personally I find _operation as a convention meaning "operation, but not used" to be more than acceptable - it preserves enough of the original to be not confusing, it acknowledges that the value is not used and it makes clear that someone has thought about it and had some reason to leave the unused parameter there anyway.

The exact means by which warnings for intentional unused parameters are suppressed is secondary though: my main point is that if we think this is a valuable warning, then it is strange to think that it is only valuable in certain types of function. For me, it should be all of ARG, or none.

personally I find _operation as a convention meaning "operation, but not used" to be more than acceptable

I don't think so. The parameter name is part of the interface (except for positional-only parameters). Changing the name in an overridden method changes the interface and thereby hurts the Liskov substitution principle.

my main point is that if we think this is a valuable warning, then it is strange to think that it is only valuable in certain types of function.

On first thought: yes. However, Liskov is not relevant for functions. That's why there will be less "false positives" than for methods and so on. IMO, it's perfectly valid to say ARG001 is the only one that does not do more damage than good. As already mentioned before I'm also fine with just merging the improvements and not enabling the check at all.

ralbertazzi · 2023-06-01T08:31:43Z

I enabled all ARG rules on the main codebase while keeping them disabled on tests. I'm more in favour of radoering's comment on preserving names to keep the Liskov principle valid rather than modifying variable names with _.

If this version doesn't find consensus yet, then I'll probably revert enabling ARG rules everywhere and just propose the improvements.

radoering

IMO the current version is fine. We have only 9 # noqa: ARG... now. If we find that the rules are too annoying, we can still disable them in the future.

I'd just revert some changes in the tests that don't make sense anymore (formatting and _ / __ arguments).

radoering · 2023-06-03T12:36:56Z

tests/console/commands/cache/test_list.py

@@ -19,7 +17,10 @@ def tester(command_tester_factory: CommandTesterFactory) -> CommandTester:


 def test_cache_list(
-    tester: CommandTester, mock_caches: None, repository_one: str, repository_two: str


This change can be reverted.

radoering · 2023-06-03T12:38:04Z

tests/console/commands/env/helpers.py

@@ -22,7 +22,7 @@ def build_venv(path: Path | str, **_: Any) -> None:
 def check_output_wrapper(
    version: Version = VERSION_3_7_1,
 ) -> Callable[[list[str], Any, Any], str]:
-    def check_output(cmd: list[str], *args: Any, **kwargs: Any) -> str:


I think I'd revert that, too

radoering · 2023-06-03T12:40:02Z

tests/console/commands/self/conftest.py

-        io: IO | None = None,
-        disable_cache: bool = False,
-    ) -> RepositoryPool:
+    def _create_pool(*_: Any, **__: Any) -> RepositoryPool:


Can we have *args: Any, **kwargs: Any here, too?

radoering · 2023-06-03T12:41:04Z

tests/console/test_application.py

@@ -40,7 +40,9 @@ def with_add_command_plugin(mocker: MockerFixture) -> None:
    mock_metadata_entry_points(mocker, AddCommandPlugin)


-def test_application_with_plugins(with_add_command_plugin: None) -> None:
+def test_application_with_plugins(


Can be reverted.

radoering · 2023-06-03T12:41:19Z

tests/console/test_application.py

@@ -50,7 +52,9 @@ def test_application_with_plugins(with_add_command_plugin: None) -> None:
    assert tester.status_code == 0


-def test_application_with_plugins_disabled(with_add_command_plugin: None) -> None:
+def test_application_with_plugins_disabled(


Can be reverted.

radoering · 2023-06-03T12:44:25Z

tests/puzzle/test_provider.py

@@ -37,7 +37,7 @@


 class MockEnv(BaseMockEnv):
-    def run(self, bin: str, *args: str, **kwargs: Any) -> str:
+    def run(self, *_: str, **__: Any) -> str:


revert or *args: str, **kwargs: Any

radoering · 2023-06-03T12:45:07Z

tests/repositories/test_single_page_repository.py

@@ -34,7 +35,7 @@ def _get_page(self, name: NormalizedName) -> SimpleRepositoryPage:
        with fixture.open(encoding="utf-8") as f:
            return SimpleRepositoryPage(self._url, f.read())

-    def _download(self, url: str, dest: Path) -> None:
+    def _download(self, *_: Any, **__: Any) -> None:


revert or *args: Any, **kwargs: Any

radoering · 2023-06-03T12:52:25Z

tests/utils/test_env.py

@@ -190,7 +190,7 @@ def build_venv(path: Path | str, **__: Any) -> None:
 def check_output_wrapper(
    version: Version = VERSION_3_7_1,
 ) -> Callable[[list[str], Any, Any], str]:
-    def check_output(cmd: list[str], *args: Any, **kwargs: Any) -> str:
+    def check_output(cmd: list[str], *_: Any, **__: Any) -> str:


radoering · 2023-06-03T12:52:48Z

tests/utils/test_env.py

@@ -936,7 +916,7 @@ def test_remove_keeps_dir_if_not_deleteable(
        side_effect=check_output_wrapper(Version.parse("3.6.6")),
    )

-    def err_on_rm_venv_only(path: Path, *args: Any, **kwargs: Any) -> None:
+    def err_on_rm_venv_only(path: Path, *_: Any, **__: Any) -> None:


radoering · 2023-06-03T12:53:28Z

tests/utils/test_env.py

@@ -1626,7 +1586,7 @@ def test_create_venv_accepts_fallback_version_w_nonzero_patchlevel(

    poetry.package.python_versions = "~3.5.1"

-    def mock_check_output(cmd: str, *args: Any, **kwargs: Any) -> str:
+    def mock_check_output(cmd: str, *_: Any, **__: Any) -> str:


Secrus · 2024-01-30T11:30:17Z

@ralbertazzi are you still interested in this? If not, please close.

abn · 2025-01-17T21:18:40Z

@ralbertazzi thank you for taking the time to work on this, propose this, and worth through feedback. However, at this point I do not think we would want this change unfortunately. In a code base like Poetry, the the noise a rule like this produces is quite high. Things might change in the future, but considering how most of this code base is written, I would not encourage this rule being activated.

For now closing. Once again, we really appreciate your time and effort.

ralbertazzi · 2025-01-17T21:28:48Z

No worries, thank you for your steady contributions :)

feat: add ARG rules

de2d1ea

radoering reviewed May 31, 2023

View reviewed changes

tests/helpers.py Outdated Show resolved Hide resolved

tests/conftest.py Show resolved Hide resolved

chore: enable ARG001 only

d6a4c1a

ralbertazzi force-pushed the refactor/remove-unused-extras branch from 035f8f2 to d6a4c1a Compare May 31, 2023 06:30

ralbertazzi changed the title ~~feat: add ARG rules~~ feat: add ARG001 rule May 31, 2023

test: ignore ARG001 rule in tests

e8d5f85

dimbleby reviewed May 31, 2023

View reviewed changes

chore: enable all ARG rules

a32786c

radoering requested changes Jun 3, 2023

View reviewed changes

neersighted self-requested a review July 7, 2023 21:49

Secrus added the status/waiting-on-response Waiting on response from author label Jan 30, 2024

abn closed this Jan 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add ARG001 rule #8045

feat: add ARG001 rule #8045

ralbertazzi commented May 30, 2023

Secrus commented May 31, 2023

finswimmer commented May 31, 2023

Secrus commented May 31, 2023

ralbertazzi commented May 31, 2023

radoering commented May 31, 2023

radoering commented May 31, 2023

ralbertazzi commented May 31, 2023

dimbleby May 31, 2023

ralbertazzi May 31, 2023

dimbleby May 31, 2023

dimbleby May 31, 2023 •

edited

Loading

ralbertazzi May 31, 2023

dimbleby May 31, 2023 •

edited

Loading

radoering Jun 1, 2023 •

edited

Loading

ralbertazzi commented Jun 1, 2023

radoering left a comment

radoering Jun 3, 2023

radoering Jun 3, 2023

radoering Jun 3, 2023

radoering Jun 3, 2023

radoering Jun 3, 2023

radoering Jun 3, 2023

radoering Jun 3, 2023

radoering Jun 3, 2023

radoering Jun 3, 2023

radoering Jun 3, 2023

Secrus commented Jan 30, 2024

abn commented Jan 17, 2025

ralbertazzi commented Jan 17, 2025

		@@ -19,7 +17,10 @@ def tester(command_tester_factory: CommandTesterFactory) -> CommandTester:


		def test_cache_list(
		tester: CommandTester, mock_caches: None, repository_one: str, repository_two: str

feat: add ARG001 rule #8045

feat: add ARG001 rule #8045

Conversation

ralbertazzi commented May 30, 2023

Secrus commented May 31, 2023

finswimmer commented May 31, 2023

Secrus commented May 31, 2023

ralbertazzi commented May 31, 2023

radoering commented May 31, 2023

radoering commented May 31, 2023

ralbertazzi commented May 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimbleby May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimbleby May 31, 2023 • edited Loading

Choose a reason for hiding this comment

radoering Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

ralbertazzi commented Jun 1, 2023

radoering 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

Secrus commented Jan 30, 2024

abn commented Jan 17, 2025

ralbertazzi commented Jan 17, 2025

dimbleby May 31, 2023 •

edited

Loading

dimbleby May 31, 2023 •

edited

Loading

radoering Jun 1, 2023 •

edited

Loading