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

docparams extension considers type comments as type documentation. #6288

Merged
merged 1 commit into from
Jun 13, 2023

Conversation

AWhetter
Copy link
Contributor

Type of Changes

Type
πŸ› Bug fix
βœ“ ✨ New feature
πŸ”¨ Refactoring
πŸ“œ Docs

Description

The docparams extension won't raise a missing-type-doc when type annotations are given. This changes means that it also won't raise a missing-type-doc when type comments are given.

Closes #6287

@coveralls
Copy link

coveralls commented Apr 13, 2022

Pull Request Test Coverage Report for Build 3677661165

  • 65 of 68 (95.59%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.003%) to 95.49%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pylint/extensions/_check_docs_utils.py 61 64 95.31%
Totals Coverage Status
Change from base Build 3676518720: -0.003%
Covered Lines: 17700
Relevant Lines: 18536

πŸ’› - Coveralls

@Pierre-Sassoulas Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Apr 13, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.14.0 milestone Apr 13, 2022
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.

Looks great already !

pylint/extensions/_check_docs_utils.py Outdated Show resolved Hide resolved
ChangeLog Outdated Show resolved Hide resolved
pylint/extensions/_check_docs_utils.py Outdated Show resolved Hide resolved
pylint/extensions/docparams.py Outdated Show resolved Hide resolved
pylint/extensions/_check_docs_utils.py Outdated Show resolved Hide resolved
@Pierre-Sassoulas
Copy link
Member

What do you think of this astroid issue pylint-dev/astroid#1508 @AWhetter ?

@DanielNoord DanielNoord removed this from the 2.14.0 milestone May 4, 2022
@Pierre-Sassoulas Pierre-Sassoulas added the Waiting on author Indicate that maintainers are waiting for a message of the author label Jul 9, 2022
@AWhetter AWhetter force-pushed the fix_6287 branch 2 times, most recently from 41593ed to 84a8f67 Compare October 14, 2022 23:31
@github-actions

This comment has been minimized.

@AWhetter AWhetter removed the Waiting on author Indicate that maintainers are waiting for a message of the author label Oct 15, 2022
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.

LGTM, thank you πŸ‘ I'm going to wait for another review if possible but this is definitely going in 2.16.0

@Pierre-Sassoulas Pierre-Sassoulas added the Needs review πŸ” Needs to be reviewed by one or multiple more persons label Oct 15, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.16.0 milestone Oct 15, 2022


def _merge_annotations(
annotations: Iterable[astroid.NodeNG], comment_annotations: Iterable[astroid.NodeNG]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably follow the style of importing from nodes instead of astroid here.

@github-actions

This comment has been minimized.

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.

The CI fail on the doc is a temporary fail in reaching a twitter link.

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.

Let's merge this πŸŽ‰

@github-actions

This comment has been minimized.

@Pierre-Sassoulas
Copy link
Member

@AWhetter the pypi 3.7 tests seems genuine, but also it's on an old interpreter, should we just ignore it ?

@AWhetter
Copy link
Contributor Author

I think the Ellipsis checking could be incorrect in Python 3.7. I'll take a look at fixing it.

@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.16.0, 2.17.0 Jan 8, 2023
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.17.0, 3.0.0 Mar 7, 2023
@codecov
Copy link

codecov bot commented May 16, 2023

Codecov Report

Merging #6288 (eb3cc1a) into main (3a741b2) will increase coverage by 0.00%.
The diff coverage is 96.92%.

❗ Current head eb3cc1a differs from pull request most recent head d64c51c. Consider uploading reports for the commit d64c51c to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #6288   +/-   ##
=======================================
  Coverage   95.82%   95.83%           
=======================================
  Files         173      173           
  Lines       18387    18428   +41     
=======================================
+ Hits        17619    17660   +41     
  Misses        768      768           
Impacted Files Coverage Ξ”
pylint/extensions/_check_docs_utils.py 93.72% <96.72%> (+0.99%) ⬆️
pylint/extensions/docparams.py 100.00% <100.00%> (ΓΈ)

... and 6 files with indirect coverage changes

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@Pierre-Sassoulas
Copy link
Member

I tried to add coverage ans force pushed some code but I was out of idea to get the last line @AWhetter :) we're very close to being able to merge

@github-actions

This comment has been minimized.

@AWhetter
Copy link
Contributor Author

@Pierre-Sassoulas Thanks for adding those tests! I've included them again and added a test that covers that final line.
The test failure seems to be happening in the main branch and is unrelated to these changes. So I think that this is ready to merge.

@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 d64c51c

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.

Amazing, thank you for covering the last missing part πŸ‘

@Pierre-Sassoulas Pierre-Sassoulas merged commit c6d2331 into pylint-dev:main Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docparams should support missing type documentation when type comments are used
4 participants