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

Fix differing param doc false positive #6980

Merged
merged 8 commits into from
Jun 23, 2022
Merged

Fix differing param doc false positive #6980

merged 8 commits into from
Jun 23, 2022

Conversation

mpernigo
Copy link
Contributor

Type of Changes

Type
✓ 🐛 Bug fix

Description

Adds handling for positional only args, like it is done for keyword only:

  • argument names are read from arguments_node.posonlyargs
  • annotations from arguments_node.posonlyargs_annotations

Closes #6950

Pierre-Sassoulas
Pierre-Sassoulas requested changes Jun 19, 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.

Look promising, thank you ! Could you add some functional tests, please ?

@coveralls
Copy link

coveralls commented Jun 19, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 2547352326

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 95.286%
Totals Coverage Status
Change from base Build 2547020639: 0.001%
Covered Lines: 16637
Relevant Lines: 17460
💛 - Coveralls

@github-actions

This comment has been minimized.

Sign in to view
@mpernigo
Copy link
Contributor Author

mpernigo commented Jun 19, 2022 •
edited
Loading

Look promising, thank you ! Could you add some functional tests, please ?

Thanks! I added some tests, please, let me know if you see something missing or wrong 👍

Just realised tests on 3.7 seems broken, checking on local 🤔

@mpernigo
Copy link
Contributor Author

Right 🤦‍♂️ Positional-only were introduced on 3.8.
Unsure what I should do, then - any idea?

@github-actions

This comment has been minimized.

Sign in to view
@Pierre-Sassoulas
Copy link
Member

You can create a file specific for python 3.8+ tests and use a configuration file like this : https://github.com/PyCQA/pylint/blob/main/tests/functional/n/none_dunder_protocols_py38.rc

@DanielNoord DanielNoord added Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer False Positive 🦟 A message is emitted but nothing is wrong with the code Waiting on author Indicate that maintainers are waiting for a message of the author labels Jun 21, 2022
spagh-eddie
spagh-eddie approved these changes Jun 21, 2022
View reviewed changes
Copy link
Contributor

@spagh-eddie spagh-eddie left a comment

Choose a reason for hiding this comment

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

I installed this PR; it fixes my issue!

@github-actions

This comment has been minimized.

Sign in to view
@mpernigo
Copy link
Contributor Author

mpernigo commented Jun 22, 2022 •
edited
Loading

👋
Tests look good now - I see also test runs are not failing brutally on Python3.7! I guess it's some progress! 🎉

So, I have some questions:

  1. I am not sure how to read results of the effect on open source repo, can I ask you to have look?
  2. I see Needs Backport was added. How should I move to address that part?

@Pierre-Sassoulas
Copy link
Member

I am not sure how to read results of the effect on open source repo, can I ask you to have look?

There is a problem with it currently I don't think your PR made this many unrelated new message appear. I'm betting on a return back to normal when the next run on main is fixed.

I see Needs Backport was added. How should I move to address that part?

This is something that we're doing ourselves, don't worry. (We're going to cherry-pick your commit on a maintainance branch once it's merged in main).

Pierre-Sassoulas
Pierre-Sassoulas requested changes Jun 22, 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.

Look pretty good already, thank you !

tests/functional/ext/docparams/docparams.py Show resolved Hide resolved
tests/functional/ext/docparams/docparams_py38.py Show resolved Hide resolved
tests/functional/ext/docparams/docparams_py38.rc Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas removed the Waiting on author Indicate that maintainers are waiting for a message of the author label Jun 22, 2022
@github-actions

This comment has been minimized.

Sign in to view
Pierre-Sassoulas
Pierre-Sassoulas approved these changes Jun 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.

Very nice fix, thank you !

doc/whatsnew/2/2.15/index.rst Outdated Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas merged commit 43a3527 into pylint-dev:main Jun 23, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.14.4 milestone Jun 23, 2022
@github-actions
Copy link
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 26d76b2

@mpernigo mpernigo deleted the fix-differing-param-doc-false-positive branch June 23, 2022 07:40
DanielNoord
DanielNoord reviewed Jun 23, 2022
View reviewed changes
doc/whatsnew/2/2.15/index.rst
@@ -30,6 +30,10 @@ Extensions
False positives fixed
=====================

* The ``differing-param-doc`` check was triggered by positional only arguments.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Pierre-Sassoulas I didn't notice this, but the changelog entry is in the wrong place. Should be 2.14.4.

Copy link
Member

Choose a reason for hiding this comment

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

I'm going to fix in the maintenance branch at release time.

@Pierre-Sassoulas Pierre-Sassoulas added Backported and removed Needs backport Needs to be cherry-picked on the current patch version by a pylint's maintainer labels Jun 29, 2022
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Jun 29, 2022
@mpernigo @Pierre-Sassoulas
Fix differing param doc false positive (pylint-dev#6980)
586cd6f 
* Read `posonly` args and annotations on `check_arguments_in_docstring`

Co-authored-by: Pierre Sassoulas <[email protected]>
Pierre-Sassoulas added a commit that referenced this pull request Jun 29, 2022
@mpernigo @Pierre-Sassoulas
Fix differing param doc false positive (#6980)
c159024 
* Read `posonly` args and annotations on `check_arguments_in_docstring`

Co-authored-by: Pierre Sassoulas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielNoord DanielNoord DanielNoord left review comments

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

@spagh-eddie spagh-eddie spagh-eddie approved these changes

Assignees
No one assigned
Labels
Backported False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Milestone
2.14.4
Development

Successfully merging this pull request may close these issues.

False positive differing-param-doc if positional-only
5 participants
@mpernigo @coveralls @Pierre-Sassoulas @DanielNoord @spagh-eddie