Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[pylint] Also emit PLR0206 for properties with variadic parameters #11200

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

AlexWaygood
Copy link
Member

Summary

Currently this rule checks the number of nonvariadic parameters a property has, but doesn't check how many variadic parameters the property has. That makes little sense to me: if a property has variadic non-self parameters, that seems just as invalid as if a property has nonvariadic non-self parameters.

This PR changes the rule so that it checks the total number of parameters the property has, rather than just the number of nonvariadic parameters. Following the new APIs added in 87929ad, this has the nice side effect of also simplifying the code slightly.

This means that we diverge from pylint's behaviour a little more in how we implement this rule, but we were already doing something slightly different. It looks like pylint for some reason only checks the number of positional-or-keyword nonvariadic parameters in a property function, and ignores the number of positional-only or keyword-only nonvariadic parameters: https://github.com/pylint-dev/pylint/blob/524f2561eda21b5593ab1c69963442031817883d/pylint/checkers/classes/class_checker.py#L1414. I don't understand why they would do that; I think that's buggy behaviour from pylint there.

Test Plan

cargo test. I added some new fixtures.

@AlexWaygood AlexWaygood added bug Something isn't working rule Implementing or modifying a lint rule labels Apr 29, 2024
Copy link
Contributor

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+6 -0 violations, +0 -0 fixes in 1 projects; 43 projects unchanged)

milvus-io/pymilvus (+6 -0 violations, +0 -0 fixes)

+ pymilvus/orm/collection.py:1114:9: PLR0206 Cannot have defined parameters for properties
+ pymilvus/orm/collection.py:1255:9: PLR0206 Cannot have defined parameters for properties
+ pymilvus/orm/collection.py:237:9: PLR0206 Cannot have defined parameters for properties
+ pymilvus/orm/collection.py:259:9: PLR0206 Cannot have defined parameters for properties
+ pymilvus/orm/collection.py:266:9: PLR0206 Cannot have defined parameters for properties
+ pymilvus/orm/partition.py:112:9: PLR0206 Cannot have defined parameters for properties

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
PLR0206 6 6 0 0 0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+6 -0 violations, +0 -0 fixes in 1 projects; 43 projects unchanged)

milvus-io/pymilvus (+6 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ pymilvus/orm/collection.py:1114:9: PLR0206 Cannot have defined parameters for properties
+ pymilvus/orm/collection.py:1255:9: PLR0206 Cannot have defined parameters for properties
+ pymilvus/orm/collection.py:237:9: PLR0206 Cannot have defined parameters for properties
+ pymilvus/orm/collection.py:259:9: PLR0206 Cannot have defined parameters for properties
+ pymilvus/orm/collection.py:266:9: PLR0206 Cannot have defined parameters for properties
+ pymilvus/orm/partition.py:112:9: PLR0206 Cannot have defined parameters for properties

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
PLR0206 6 6 0 0 0

@AlexWaygood
Copy link
Member Author

ℹ️ ecosystem check detected linter changes. (+6 -0 violations, +0 -0 fixes in 1 projects; 43 projects unchanged)

These all look like true positives to me. The affected project defines a bunch of properties with **kwargs variadic parameters, but no arguments are ever passed to those parameters as far as I can see. The property definitions would make more sense without the **kwargs parameters, even if the code works fine today without any exception being raised.

@dhruvmanila
Copy link
Member

dhruvmanila commented Apr 30, 2024

I think these changes are correct but it would be useful to get a second opinion. I'd also look at why Pylint doesn't trigger on them. Is there an issue in Pylint which might give us an insight here?

(Edit: Pyright -> Pylint)

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Apr 30, 2024

I'd also look at why Pylint doesn't trigger on them. Is there an issue in Pylint which might give us an insight here?

Yeah, I'll do some digging and check to see if there's a reason why pylint only checks for the number of positional-or-keyword parameters.

@AlexWaygood
Copy link
Member Author

The results of digging:

I confirmed locally that running pylint on the original motivating example for the pylint check given in pylint-dev/pylint#3006 (comment) does not actually trigger a violation of the pylint rule (because the example has variadic non-self parameters rather than nonvariadic non-self parameters). I checked using pylint==3.1.0.

@AlexWaygood AlexWaygood requested a review from carljm April 30, 2024 08:42
@AlexWaygood
Copy link
Member Author

I think these changes are correct but it would be useful to get a second opinion.

@carljm, could you possibly take a look?

@dhruvmanila
Copy link
Member

Thanks for the details. It's good to go from my side 👍

@AlexWaygood
Copy link
Member Author

I opened a pylint issue here to let them know about the bug on their side: pylint-dev/pylint#9584

@AlexWaygood
Copy link
Member Author

The pylint maintainers have added labels to pylint-dev/pylint#9584 which indicate that they agree there's a bug on their side, so I'll merge this now 👍

Thanks for the review @dhruvmanila!

@AlexWaygood AlexWaygood merged commit 21d824a into main Apr 30, 2024
19 checks passed
@AlexWaygood AlexWaygood deleted the pylint-property branch April 30, 2024 10:59
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants