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
issue9255 - Detect FIXME words in docstring #9281
base: main
Are you sure you want to change the base?
Conversation
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.
This looks really good already!
You'll also need to add a documentation example. There is a CI check that sort of tells you what's wrong. Please let us know if you need more help figuring out what to do: that means we should update the error message of that CI step.
@@ -73,6 +73,11 @@ class DocStringChecker(_BasicChecker): | |||
"docstring.", | |||
{"old_names": [("C0111", "missing-docstring")]}, | |||
), | |||
"C0150": ( | |||
"%s", |
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.
Could we make this message a little more descriptive? Something like:
"%s", | |
"Disallowed word in docstring: '%s'", |
I know the original fixme
message doesn't have this but I don't think that's nice π
fixme_words = self.linter.config.notes | ||
for word in fixme_words: |
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.
fixme_words = self.linter.config.notes | |
for word in fixme_words: | |
for word in self.linter.config.notes: |
# pylint: disable=unnecessary-pass, consider-using-f-string | ||
|
||
""" | ||
Test fixme in docstrings. | ||
""" |
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.
Could you switch the comment and the docstring around?
# fixme-in-docstring | ||
def func3(): # [fixme-in-docstring] | ||
""" | ||
XXX: Implement |
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.
Could you add a test where the FIXME
is not the first word of the line? And also one where it is not surrounded by spaces?
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.
If it's a new check, we might want to disable it by default. Not sure it warrant to create a new message by the way. If we extends fixme
then adding an option to raise on docstring (false by default) would be nice. What do you think @DanielNoord ?
@@ -73,6 +73,11 @@ class DocStringChecker(_BasicChecker): | |||
"docstring.", | |||
{"old_names": [("C0111", "missing-docstring")]}, | |||
), | |||
"C0150": ( |
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.
I would expect this message to be in the same checker than fixme
. Maybe even the same message.
"C0150": ( | ||
"%s", | ||
"fixme-in-docstring", | ||
"Used when a warning note as FIXME or XXX is detected in docstring.", |
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.
Imo should be a rational (why it's bad) and not when it's raised. (I know the current descriptions are a bad example of that.)
I'm fine with adding it to |
Ok! Should I change to integrate it to |
Yes, I think pylint would be more consistent if it's still a pylint/pylint/checkers/design_analysis.py Lines 292 to 407 in 37081fd
|
Type of Changes
Description
Added checking in docstring. It checks for FIXME words (TODO, FIXME, XXX) in a docstring and give warnings.
Closes #9255