[pylint] Implement `too-many-public-methods` rule (PLR0904) #6179

jelly · 2023-07-30T13:57:07Z

Implement https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/too-many-public-methods.html

Confusingly the rule page mentions a max of 7 while in practice it is 20. https://github.com/search?q=repo%3Apylint-dev%2Fpylint+max-public-methods&type=code

Summary

Implement pylint's R0904

Test Plan

Unit tests.

github-actions · 2023-07-30T14:06:45Z

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

jelly · 2023-07-31T09:14:55Z

python scripts/generate_mkdocs.py succeeds locally for me, so I am not sure what's going on in CI.

crates/ruff/src/rules/pylint/rules/too_many_public_methods.rs

konstin

lgtm, needs a review from @charliermarsh

jelly · 2023-08-25T15:27:20Z

@charliermarsh gentle ping.

charliermarsh · 2023-08-27T15:17:11Z

crates/ruff/src/rules/pylint/rules/too_many_public_methods.rs

+    let methods = body
+        .iter()
+        .filter(|stmt| match stmt {
+            Stmt::FunctionDef(ast::StmtFunctionDef { name, .. }) => !name.starts_with('_'),


Can we use method_visibility here from analyze::visibility? It takes into account dunders and a few other cases.

method_visibility is not public enough (pub crate) it seems to be used here, should it be? Or should it mimmick what the module has an add a to_visibility?

I think it's okay to make it pub and use it here.

Applied, the changes. Please have a look.

charliermarsh · 2023-08-27T15:17:47Z

crates/ruff/src/rules/pylint/rules/too_many_public_methods.rs

+    class_def: &ast::StmtClassDef,
+    max_methods: usize,
+) {
+    let ast::StmtClassDef { name, body, .. } = class_def;


Do you know if this rule should be applied to private classes? What about to classes in private modules? Do you mind checking how Pylint treats those?

What would a "private module" be in Python? _module.py ? As far as I can see the rule counts the methods except dunders. Well I assume it also ignores def _foo() as that would be private by convention in Python.

Yeah, something like _module.py, which would be considered private based on pydocstyle's definitions of public vs. private visibility for functions.

Implement https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/too-many-public-methods.html

jelly force-pushed the pylint-too-many-public-methods branch 3 times, most recently from bc7a27a to f5f3066 Compare July 31, 2023 09:01

konstin reviewed Aug 1, 2023

View reviewed changes

crates/ruff/src/rules/pylint/rules/too_many_public_methods.rs Outdated Show resolved Hide resolved

crates/ruff/src/rules/pylint/rules/too_many_public_methods.rs Outdated Show resolved Hide resolved

jelly force-pushed the pylint-too-many-public-methods branch from f5f3066 to 0629206 Compare August 1, 2023 16:23

konstin requested a review from charliermarsh August 3, 2023 09:45

jelly requested a review from konstin August 8, 2023 18:15

konstin approved these changes Aug 11, 2023

View reviewed changes

charliermarsh reviewed Aug 27, 2023

View reviewed changes

jelly force-pushed the pylint-too-many-public-methods branch from 0629206 to 5a075bc Compare August 29, 2023 20:23

charliermarsh self-requested a review August 29, 2023 23:15

jelly force-pushed the pylint-too-many-public-methods branch from 5a075bc to 385a1ab Compare August 30, 2023 08:06

[pylint] Implement too-many-public-methods rule (PLR0904)

a66ec47

Implement https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/too-many-public-methods.html

jelly force-pushed the pylint-too-many-public-methods branch from 385a1ab to a66ec47 Compare August 30, 2023 08:24

Merge branch 'main' into pylint-too-many-public-methods

3d1ae64

charliermarsh force-pushed the pylint-too-many-public-methods branch from 7b1ca54 to 1dd0bd4 Compare September 14, 2023 00:35

charliermarsh added the rule Implementing or modifying a lint rule label Sep 14, 2023

charliermarsh added 2 commits September 13, 2023 20:35

Tweaks

1dd0bd4

Move to preview

ce7fc9e

charliermarsh enabled auto-merge (squash) September 14, 2023 00:40

charliermarsh merged commit 04183b0 into astral-sh:main Sep 14, 2023
16 checks passed

jelly deleted the pylint-too-many-public-methods branch October 24, 2024 12:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pylint] Implement `too-many-public-methods` rule (PLR0904) #6179

[pylint] Implement `too-many-public-methods` rule (PLR0904) #6179

jelly commented Jul 30, 2023

github-actions bot commented Jul 30, 2023 •

edited

Loading

jelly commented Jul 31, 2023

konstin left a comment

jelly commented Aug 25, 2023

charliermarsh Aug 27, 2023

jelly Aug 28, 2023

charliermarsh Aug 28, 2023

jelly Sep 13, 2023

charliermarsh Aug 27, 2023

jelly Aug 28, 2023

charliermarsh Aug 28, 2023

[pylint] Implement too-many-public-methods rule (PLR0904) #6179

[pylint] Implement too-many-public-methods rule (PLR0904) #6179

Conversation

jelly commented Jul 30, 2023

Summary

Test Plan

github-actions bot commented Jul 30, 2023 • edited Loading

PR Check Results

Ecosystem

jelly commented Jul 31, 2023

konstin left a comment

Choose a reason for hiding this comment

jelly commented Aug 25, 2023

charliermarsh Aug 27, 2023

Choose a reason for hiding this comment

jelly Aug 28, 2023

Choose a reason for hiding this comment

charliermarsh Aug 28, 2023

Choose a reason for hiding this comment

jelly Sep 13, 2023

Choose a reason for hiding this comment

charliermarsh Aug 27, 2023

Choose a reason for hiding this comment

jelly Aug 28, 2023

Choose a reason for hiding this comment

charliermarsh Aug 28, 2023

Choose a reason for hiding this comment

[pylint] Implement `too-many-public-methods` rule (PLR0904) #6179

[pylint] Implement `too-many-public-methods` rule (PLR0904) #6179

github-actions bot commented Jul 30, 2023 •

edited

Loading