Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Move no-self-use to optional extension #6448

Merged
merged 15 commits into from
May 5, 2022
Merged

Move no-self-use to optional extension #6448

merged 15 commits into from
May 5, 2022

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Apr 23, 2022

Description

Create new optional extension for no-self-use and move check.

Closes #5502

@cdce8p cdce8p added this to the 2.14.0 milestone Apr 23, 2022
@coveralls
Copy link

coveralls commented Apr 23, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 2277033644

  • 49 of 49 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.009%) to 95.287%
Totals Coverage Status
Change from base Build 2276629135: 0.009%
Covered Lines: 15951
Relevant Lines: 16740
💛 - Coveralls

Pierre-Sassoulas
Pierre-Sassoulas approved these changes Apr 23, 2022
View reviewed changes
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great 👍 The fact that we had so much disable for it in our own code, is proof that it should not be enabled by default. I like that you actually made a standalone checker, independent and fully typed.

Pierre-Sassoulas
Pierre-Sassoulas requested changes Apr 23, 2022
View reviewed changes
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like there's no functional test specific to this check, and the coverage is low for some part. Would you mind recreating a covering functional test ?

@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component Checkers Related to a checker labels Apr 27, 2022
@cdce8p cdce8p force-pushed the move_no-self-use branch from ac98a90 to a0992e1 Compare May 5, 2022 10:45
@jacobtylerwalls jacobtylerwalls mentioned this pull request May 5, 2022
Merged
Pierre-Sassoulas
Pierre-Sassoulas requested changes May 5, 2022
View reviewed changes
doc/whatsnew/2.14.rst Show resolved Hide resolved
ChangeLog Show resolved Hide resolved
pylint/extensions/no_self_use.py Show resolved Hide resolved
tests/functional/ext/no_self_use/no_self_use.py Show resolved Hide resolved
Pierre-Sassoulas
Pierre-Sassoulas approved these changes May 5, 2022
View reviewed changes
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@DanielNoord
Copy link
Collaborator

For some reason duplicate-code gets triggered on this branch and on some other PRs but not on main 😓

@cdce8p
Copy link
Member Author

cdce8p commented May 5, 2022

For some reason duplicate-code gets triggered on this branch and on some other PRs but not on main 😓

Saw that one too. A bit strange tbh. I can't reproduce the warnings locally although they are valid. I've added a disabled comment for the one instance directly caused by this change. This should allow us to move forward with it. The rest can be addressed sometime later.

@cdce8p cdce8p merged commit 66fcbeb into pylint-dev:main May 5, 2022
@cdce8p cdce8p deleted the move_no-self-use branch May 5, 2022 16:47
@DanielNoord
Copy link
Collaborator

@cdce8p The CI job still fails. Do you want to do a follow-up to unblock main?

@cdce8p
Copy link
Member Author

cdce8p commented May 5, 2022

@cdce8p The CI job still fails. Do you want to do a follow-up to unblock main?

I'll open a PR shortly. Interestingly, now main also fails. Honestly not sure why it hasn't previously.
https://github.com/PyCQA/pylint/runs/6310323884?check_suite_focus=true#step:9:34

@cdce8p cdce8p mentioned this pull request May 5, 2022
Merged
The-Compiler added a commit to qutebrowser/qutebrowser that referenced this pull request Jun 8, 2022
@The-Compiler
pylint: Remove outdated disables
6347d71 
Looks like 2.14 added bad-option-value:
https://pylint.pycqa.org/en/latest/whatsnew/2/2.14/full.html

locally-enabled is long gone:
pylint-dev/pylint#2442

and so is bad-continuation:
pylint-dev/pylint#3571

no-self-use was moved to an extension in this release:
pylint-dev/pylint#6448
tbekolay added a commit to nengo/nengo-bones that referenced this pull request Jun 8, 2022
@tbekolay
Remove 'no-self-use' pylint disable
f503727 
It was made an optional checker that must be installed in 2.14.0:
pylint-dev/pylint#6448
tbekolay added a commit to nengo/nengo-bones that referenced this pull request Jun 8, 2022
@tbekolay
Remove 'no-self-use' pylint disable
6fe056c 
It was made an optional checker that must be installed in 2.14.0:
pylint-dev/pylint#6448
@jacobtylerwalls jacobtylerwalls mentioned this pull request Jun 12, 2022
Merged
@kemingy kemingy mentioned this pull request Jun 15, 2022
Merged
@DanielNoord DanielNoord mentioned this pull request Jun 29, 2022
Open
@tigattack tigattack mentioned this pull request Oct 11, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas approved these changes

Assignees
No one assigned
Labels
Checkers Related to a checker Enhancement ✨ Improvement to a component
Projects
None yet
Milestone
2.14.0
Development

Successfully merging this pull request may close these issues.

Move no-self-use to optional checker
4 participants
@cdce8p @coveralls @DanielNoord @Pierre-Sassoulas