-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Respect docstring-min-length in docparams extension #10104
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10104 +/- ##
=======================================
Coverage 95.87% 95.87%
=======================================
Files 176 176
Lines 19137 19145 +8
=======================================
+ Hits 18347 18355 +8
Misses 790 790
🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for opening a PR ! Would you mind adding functional tests (https://github.com/pylint-dev/pylint/tree/main/tests/functional/ext/docparams) and a changelog (https://github.com/pylint-dev/pylint/tree/main/doc/whatsnew/fragments) for this please ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good thank you ! Any reasons not to create a reusable function for the new code in the doc param class (copy pasted thrice as far as I understand) ?
Don't mind the fail of the CI this is due to python/cpython#125415 in 3.13.1 we're going to be able to rebase on main once it's fixed. |
b794ef3
to
c8d7ec9
Compare
Sorry for a lot of pushes 😅 |
Something not related to my changes is giving an error on pre-commit checks |
Sorry about that it's going to be fixed in #10165 |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The added functional tests look amazing ! I authorized the workflow, let's see how it goes.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great first merge request @berkersal. Thank you for the great functional tests. I asked for some clarification on the intent of the MR, there seem to be some contradiction between the title and the test cases (and the description did not permit to clarify for me).
tests/functional/ext/docparams/raise/missing_raises_doc_required_min_length.py
Show resolved
Hide resolved
Even though visit_functiondef is checking for docstring-min-length, it is not enough. This commit fixes the issue by adding the same check to visit_raise, visit_return and visit_yield
for more information, see https://pre-commit.ci
Sorry that I've reverted some changes accidentally last week, now it should be the correct code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome thanks!
(cherry picked from commit 7f5e996)
(cherry picked from commit 7f5e996) Co-authored-by: Berker ŞAL <[email protected]>
Even though visit_functiondef is checking for docstring-min-length, it is not enough. This commit fixes the issue by adding the same check to visit_raise and visit_yield
If there is a better way of implementing this, please go forward. I am just providing a working fix to this problem.
Type of Changes
Description
Refs #XXXX
Closes #XXXX