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.

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

[unused-private-member] add logic for checking nested functions #4709

Merged
merged 7 commits into from Aug 1, 2021

Conversation

yushao2
Copy link
Collaborator

@yushao2 yushao2 commented Jul 14, 2021

Steps

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature or an important bug fix, add a What's New entry in
    doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Description

Add logic for checking (private) nested functions in a class which was causing a false-negative as reported in
#4673

also, improve error message for nested functions

Type of Changes

Type
🐛 Bug fix
📜 Docs

Related Issue

Closes #4673

@yushao2 yushao2 added the False Positive 🦟 A message is emitted but nothing is wrong with the code label Jul 14, 2021
@yushao2 yushao2 added this to the 2.9.4 milestone Jul 14, 2021
@coveralls
Copy link

coveralls commented Jul 14, 2021

Coverage Status

Coverage increased (+0.005%) to 92.264% when pulling 1d09bc8 on yushao2:fix-unused-private-member-4673 into 8ceb26d on PyCQA:main.

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.

👍

pylint/checkers/classes.py Outdated Show resolved Hide resolved
pylint/checkers/classes.py Outdated Show resolved Hide resolved
pylint/checkers/classes.py Outdated Show resolved Hide resolved
pylint/checkers/classes.py Outdated Show resolved Hide resolved
pylint/checkers/classes.py Outdated Show resolved Hide resolved
@yushao2 yushao2 force-pushed the fix-unused-private-member-4673 branch from b659153 to f640714 Compare July 18, 2021 14:47
@yushao2 yushao2 requested a review from cdce8p July 18, 2021 15:05
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

Some last comments

pylint/checkers/classes.py Outdated Show resolved Hide resolved
ChangeLog Outdated Show resolved Hide resolved
pylint/checkers/classes.py Outdated Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.9.4, 2.9.5, 2.9.6 Jul 19, 2021
@yushao2 yushao2 force-pushed the fix-unused-private-member-4673 branch from 1976115 to aef8e22 Compare July 25, 2021 05:19
@yushao2 yushao2 requested a review from cdce8p July 25, 2021 07:31
@yushao2
Copy link
Collaborator Author

yushao2 commented Jul 25, 2021

@cdce8p busy week for me, so sorry for the lack of response. Have updated my PR based on comments :)

ChangeLog Outdated Show resolved Hide resolved
pylint/checkers/classes.py Outdated Show resolved Hide resolved
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

@yushao2 Don't worry about it! Just let us know if we can help.

Left one more comment, but after that it will be good for merging.

pylint/checkers/classes.py Outdated Show resolved Hide resolved
pylint/checkers/classes.py Outdated Show resolved Hide resolved
saimonation added a commit to ctera/ctera-python-sdk that referenced this pull request Jul 26, 2021
saimonation added a commit to ctera/ctera-python-sdk that referenced this pull request Jul 26, 2021
* Modify Portal servers
* Enable or disable Portal syslog
* Add ignore until false-positive is fixed
- pylint-dev/pylint#4709
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.9.6, 2.10.1 Jul 28, 2021
@yushao2 yushao2 requested a review from cdce8p August 1, 2021 11:23
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

Almost there


function_repr = f"{outer_level_names}{function_def.name}({function_def.args.as_string()})"
Copy link
Member

Choose a reason for hiding this comment

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

I really liked this line. It helped make the code more readable. In contrast to having a long f-string as an argument.

pylint/checkers/classes.py Outdated Show resolved Hide resolved
@cdce8p cdce8p modified the milestones: 2.10.1, 2.10.0 Aug 1, 2021
@yushao2 yushao2 force-pushed the fix-unused-private-member-4673 branch from 7423a35 to a4198cd Compare August 1, 2021 11:39
@cdce8p
Copy link
Member

cdce8p commented Aug 1, 2021

@yushao2 Did you see my latest comments?

@yushao2
Copy link
Collaborator Author

yushao2 commented Aug 1, 2021

@yushao2 Did you see my latest comments?

@yushao2 Did you see my latest comments?

oops, did i miss out anything? I only saw the latest 2 comments on the lstrip, and to undo the removal of the temp variable that's holding the function's representation

Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

LGTM

@cdce8p
Copy link
Member

cdce8p commented Aug 1, 2021

oops, did i miss out anything? I only saw the latest 2 comments on the lstrip, and to undo the removal of the temp variable that's holding the function's representation

No, is fine now! After you forced-pushed some changes, I wasn't sure if you had seen my comment. But apparently you have 😉 Anyway, thank you for working on it!

@yushao2
Copy link
Collaborator Author

yushao2 commented Aug 1, 2021

LGTM

thanks! opened #4782 and #4783 earlier too related to the same checker for your review :)

@yushao2
Copy link
Collaborator Author

yushao2 commented Aug 1, 2021

oops, did i miss out anything? I only saw the latest 2 comments on the lstrip, and to undo the removal of the temp variable that's holding the function's representation

No, is fine now! After you forced-pushed some changes, I wasn't sure if you had seen my comment. But apparently you have wink Anyway, thank you for working on it!

yeah my personal dev machine is a little messy due to the massive changes in the codebase so have been rebasing my work and been typing -f more than i should have hahaha

@yushao2 yushao2 merged commit 2ad8247 into pylint-dev:main Aug 1, 2021
@yushao2 yushao2 deleted the fix-unused-private-member-4673 branch August 1, 2021 12:13
cdce8p added a commit to cdce8p/pylint that referenced this pull request Aug 1, 2021
Squashed commit of the following:

commit 49c4bba
Author: Yu Shao, Pang <p.yushao2@gmail.com>
Date:   Sun Aug 1 19:56:51 2021 +0800

    Fix crash for `unused-private-member` when there are nested attributes

commit 2ad8247
Merge: 8ceb26d 1d09bc8
Author: yushao2 <36848472+yushao2@users.noreply.github.com>
Date:   Sun Aug 1 20:13:05 2021 +0800

    Merge pull request pylint-dev#4709 from yushao2/fix-unused-private-member-4673

    [unused-private-member] add logic for checking nested functions

commit 1d09bc8
Author: Yu Shao, Pang <p.yushao2@gmail.com>
Date:   Sun Aug 1 20:03:42 2021 +0800

    update pr based on review

commit a4198cd
Author: Yu Shao, Pang <p.yushao2@gmail.com>
Date:   Sun Aug 1 19:21:36 2021 +0800

    Update pr based on review

commit c8b2cbb
Author: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date:   Sun Jul 25 05:20:42 2021 +0000

    [pre-commit.ci] auto fixes from pre-commit.com hooks

    for more information, see https://pre-commit.ci

commit 4ffea0b
Author: Yu Shao, Pang <p.yushao2@gmail.com>
Date:   Sun Jul 25 13:12:29 2021 +0800

    Update pr based on review

commit e4d6243
Author: Marc Mueller <30130371+cdce8p@users.noreply.github.com>
Date:   Sun Jul 18 17:23:31 2021 +0200

    Remove empty line

commit cce5833
Author: Yu Shao, Pang <p.yushao2@gmail.com>
Date:   Sun Jul 18 22:40:54 2021 +0800

    update PR based on comments

commit 712dc2b
Author: Yu Shao, Pang <p.yushao2@gmail.com>
Date:   Wed Jul 14 16:42:25 2021 +0800

    [unused-private-member] add logic for checking nested functions

    also, improve error message for nested functions

commit 8ceb26d
Author: Michal Vasilek <michal@vasilek.cz>
Date:   Sun Aug 1 08:14:58 2021 +0200

    Fix IsADirectoryError in tests/lint/unittest_lint (pylint-dev#4781)

    pylintd is a directory, so os.remove throws IsADirectoryError

commit a31e6bc
Author: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Date:   Sat Jul 31 11:21:46 2021 +0200

    Add documentation for useless-suppression

    Closes pylint-dev#4757

commit b71be8a
Author: Mark Byrne <31762852+mbyrnepr2@users.noreply.github.com>
Date:   Fri Jul 30 20:21:02 2021 +0200

    Handle has-a relationships for type-hinted arguments in class diagrams (pylint-dev#4745)

    * Pyreverse - Show class has-a relationships inferred from type-hints

    Closes pylint-dev#4744

    Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

commit 5e5f48d
Author: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
Date:   Thu Jul 29 23:44:30 2021 +0200

    Add ``forgotten-debug-statement`` checker (pylint-dev#4771)

    * Add ``no-breakpoint`` checker this adds a checker for calls to ``breakpoint()``, ``pdb.set_trace()``, or ``sys.breakpointhook()``.

    Closes pylint-dev#3692

    Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>
    Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False Positive on Unused-private-member for nested function
4 participants