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 missing-param-doc FP #7878

Merged
merged 4 commits into from Dec 4, 2022
Merged

Fix missing-param-doc FP #7878

merged 4 commits into from Dec 4, 2022

Conversation

clavedeluna
Copy link
Collaborator

Type of Changes

Type
✓ 🐛 Bug fix

Description

As mentioned in linked issue, if this is too a hacky solution, we can reject the PR and attempt to update the re_param_line regex.

Closes #7827

@coveralls
Copy link

coveralls commented Nov 30, 2022 •
edited

Pull Request Test Coverage Report for Build 3613572879

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 82 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.05%) to 95.482%
Files with Coverage Reduction New Missed Lines %
pylint/extensions/typing.py 4 97.67%
pylint/checkers/utils.py 22 95.25%
pylint/checkers/typecheck.py 25 96.24%
pylint/checkers/classes/class_checker.py 31 95.07%
Totals Coverage Status
Change from base Build 3586260495: 0.05%
Covered Lines: 17645
Relevant Lines: 18480
💛 - Coveralls

@Pierre-Sassoulas Pierre-Sassoulas added False Positive 🦟 A message is emitted but nothing is wrong with the code backport maintenance/2.15.x labels Nov 30, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.15.8 milestone Nov 30, 2022
@github-actions

This comment has been minimized.

Sign in to view
@clavedeluna clavedeluna added the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Dec 1, 2022
Pierre-Sassoulas
Pierre-Sassoulas previously approved these changes Dec 4, 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 does feel hacky, but it's simple and I don't see what could go wrong.

@Pierre-Sassoulas Pierre-Sassoulas added the Blocked 🚧 Blocked by a particular issue label Dec 4, 2022
@Pierre-Sassoulas
Copy link
Member

Blocked by tests not being launched on backported branch that needs fixing so I don't have to rebase this later. Also another review would be welcome.

DanielNoord
DanielNoord requested changes Dec 4, 2022
View reviewed changes
pylint/extensions/_check_docs_utils.py Outdated
@@ -644,7 +644,7 @@ def match_param_docs(self) -> tuple[set[str], set[str]]:
entries = self._parse_section(self.re_param_section)
entries.extend(self._parse_section(self.re_keyword_param_section))
for entry in entries:
match = self.re_param_line.match(entry)
match = self.re_param_line.match(entry.replace("\\", ""))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why don't we change self._re_param_line? We can include \\ there.

In any case, we are also replacing \\a bit further down here so that should be fixed as well then if we continue down the current road.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have no idea how to change _re_param_line, it's really difficult to read. If you can, plz do paste your regex here!

Copy link
Collaborator

Choose a reason for hiding this comment

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

True, I'd rather not touch it as well, but just replacing all \\ seems error prone as well.
I think it should be as easy as allowing both \w and \\ in _re_param_line. You can check this with regex101.com

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should be as easy

Oh I tried, I really tried 🤣 , but nothing quite "works". As soon as you try to add some variation of (\)* or something (Tried lots of ways), it breaks other tests!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In any case, we are also replacing \a bit further down here so that should be fixed as well then if we continue down the current road.

reasonable and doesn't seem to break tests

DanielNoord
DanielNoord requested changes Dec 4, 2022
View reviewed changes
Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

I fixed the regex and made some suggestions! 😄

doc/whatsnew/fragments/7827.false_positive Outdated Show resolved Hide resolved
tests/functional/ext/docparams/parameter/missing_param_doc_required_Google.py Outdated Show resolved Hide resolved
pylint/extensions/_check_docs_utils.py Outdated Show resolved Hide resolved
@clavedeluna @DanielNoord
Apply suggestions from code review
6097f34 
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2022

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

This comment was generated for commit 6097f34

@DanielNoord DanielNoord merged commit 1913635 into pylint-dev:main Dec 4, 2022
github-actions bot pushed a commit that referenced this pull request Dec 4, 2022
@clavedeluna @github-actions
Fix missing-param-doc for escaped underscores (#7878)
c18833b 
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
(cherry picked from commit 1913635)
@github-actions github-actions bot mentioned this pull request Dec 4, 2022
Merged
@Pierre-Sassoulas Pierre-Sassoulas removed Blocked 🚧 Blocked by a particular issue Needs review 🔍 Needs to be reviewed by one or multiple more persons labels Dec 4, 2022
Pierre-Sassoulas pushed a commit that referenced this pull request Dec 4, 2022
@clavedeluna @Pierre-Sassoulas
Fix missing-param-doc for escaped underscores (#7878)
85706f4 
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
(cherry picked from commit 1913635)
Pierre-Sassoulas pushed a commit that referenced this pull request Dec 4, 2022
@clavedeluna @Pierre-Sassoulas
Fix missing-param-doc for escaped underscores (#7878)
19c0534 
Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
(cherry picked from commit 1913635)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DanielNoord DanielNoord DanielNoord approved these changes

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas left review comments

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

Successfully merging this pull request may close these issues.

False positive missing-param-doc when escaping underscore in docstring
4 participants
@clavedeluna @coveralls @Pierre-Sassoulas @DanielNoord