-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement too-many-positional-arguments
#9842
Conversation
"""The max positional arguments default is 5.""" | ||
|
||
# +1: [too-many-arguments, too-many-positional]] | ||
def take_five_args(self, a, b, c, d, e): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I welcome input on making this more user-friendly--I was just hammering this out quickly.
This comment has been minimized.
This comment has been minimized.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9842 +/- ##
=======================================
Coverage 95.79% 95.80%
=======================================
Files 174 174
Lines 18921 18932 +11
=======================================
+ Hits 18126 18137 +11
Misses 795 795
|
This comment has been minimized.
This comment has been minimized.
too-many-positional
too-many-positional-arguments
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great !
@@ -1 +0,0 @@ | |||
Reserved message name, not yet implemented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably need some explanation about how it's hard to remember the order of everything if there's a lot of positional and thus it's a bad API. (We can take some inspiration from what Ruff says)
"msgid/symbol pair reserved for compatibility with ruff, " | ||
"see https://github.com/astral-sh/ruff/issues/8946.", | ||
"Too many positional arguments (%s/%s)", | ||
"too-many-positional-arguments", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also prefer the new name. But Ruff chose the other one and we originally created this message in anticipation for compatibility with ruff. Is it worth the possible confusion ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can easily rename. ruff
only supports codes for now, so renaming is just a change in output and documentation. That should be fine for them? Considering astral-sh/ruff#8946 was merged by @charliermarsh perhaps they can confirm that a rename should be fine for them as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They fixed it in 0.6.1 / astral-sh/ruff#12905. There's a comment saying we should have named it "too-many-positional-parameters
(astral-sh/ruff#12619 (comment)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get it. But leaving it this way maintains the rhyme between the existing max-args
setting and the new max-positional-arguments
setting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah and the churn in ruff and pylint if we change it again would be pretty bad. (tbh I didn't consciously make this distinction between arguments/parameters internally myself)
setuptools version is pinned to 70.3.0 to resolve an issue related to this ticket: pypa/setuptools#4483 pylint version is forced to >=3.3.0, where max-positional-arguments functionality has been added and max-positional-arguments is set to 16: pylint-dev/pylint#9842 Changes in functional test test_stateless_sec_group_list_find because sec group lists from a sec group can be ordered differently each time a request is sent to obtain them. Changed custom_mtu_size default value from 1350 to 1300 because some tests started failing on some jobs with: "Requested MTU is too big, maximum is 1314" Change-Id: Ie92d9a2f4e0dd08aeadfd720bdf4963b532decf3
Type of Changes
Description
Closes #9099