-
-
Notifications
You must be signed in to change notification settings - Fork 405
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
quality: Docstring cleanup for pydocstyle prep #1978
Conversation
3ed9157
to
7f95c61
Compare
To tl;dr IRC:
|
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.
First round of review. I still think it's too early to add a strong docstring check, and I still think these little changes don't actually improve the documentation.
@@ -131,7 +131,7 @@ def scheduler(self): | |||
|
|||
@property | |||
def command_groups(self): | |||
"""A mapping of plugin names to lists of their commands. | |||
"""Map of plugin names to lists of their commands. |
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'm really not keen on changing @property
's docstring: from the documentation reader point of view, these are all attributes, and not methods.
@@ -853,7 +853,7 @@ def _update_running_triggers(self, running_triggers): | |||
t for t in running_triggers if t.is_alive()] | |||
|
|||
def on_scheduler_error(self, scheduler, exc): | |||
"""Called when the Job Scheduler fails. | |||
"""Handle failed Job Scheduler. |
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 is a clear case where compliance with a tool doesn't improve the quality, but decrease it. Two options here:
- don't change the docstring
- add the information in the description about when this method is called
When is the important information, not what it does.
@@ -933,7 +933,7 @@ def _nick_blocked(self, nick): | |||
return False | |||
|
|||
def _shutdown(self): | |||
"""Internal bot shutdown method.""" | |||
"""Shut down the bot.""" |
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.
Since it's a private method, it's usually suggested to replace docstring with inline comments, such as this:
"""Shut down the bot.""" | |
# Internal bot shutdown method. |
@@ -145,7 +148,7 @@ def __init__(self, filename, validate=True): | |||
|
|||
@property | |||
def homedir(self): | |||
"""The config file's home directory. | |||
"""Home directory for config file. |
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.
Ditto @ "property are not method from the reader pov".
@@ -151,21 +152,21 @@ def initiate_connect(self, host, port, source_address): | |||
self.handle_close() | |||
|
|||
def handle_connect(self): | |||
"""Called when the active opener's socket actually makes a connection.""" | |||
"""Handle active opener's new accepted connection.""" |
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.
Lost details here.
""" | ||
Send an ACTION (/me) to a given channel or nick. Can only be done in | ||
privmsg by an admin. | ||
"""Send an ACTION (/me) to a given channel or nick. |
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.
"""Send an ACTION (/me) to a given channel or nick. | |
"""Send an ``ACTION`` (``/me``) to a given channel or nick. |
""" | ||
Command to op users in a room. If no nick is given, | ||
Sopel will op the nick who sent the command | ||
"""Op users in a room. |
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.
Maybe we should change room
to channel
?
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.
"""Op users in a room. | |
"""Op users in a channel. |
""" | ||
Command to deop users in a room. If no nick is given, | ||
Sopel will deop the nick who sent the command | ||
"""Deop users in a room. |
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.
"""Deop users in a room. | |
"""Deop users in a channel. |
""" | ||
Command to voice users in a room. If no nick is given, | ||
Sopel will voice the nick who sent the command | ||
"""Voice users in a room. |
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.
"""Voice users in a room. | |
"""Voice users in a channel. |
""" | ||
Command to devoice users in a room. If no nick is given, | ||
Sopel will devoice the nick who sent the command | ||
"""Devoice users in a room. |
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.
"""Devoice users in a room. | |
"""Devoice users in a channel. |
Could revisit this now that most of the stuff to be removed in 8.0 is gone. Even so, I think we're best off with a transitional period where this check is a separate There are so many merge conflicts now that it's probably easier to add the plugin and apply fixes from scratch. Since I literally asked to use this in #1765, I'm certainly happy to be the one who goes over everything again. (Could be therapeutic after all my end-of-year stuff is done at work.) |
We're still going to do this, but in a much less invasive way. The sane approach is to first start getting in the habit of linting everything we touch with the new rules and fixing any style violations. We can slowly work our way up to hardline enforcement at the |
Description
Adds flake8-docstrings and starts cleanup for putting it in use.
First commit is only requirements, config, spacing changes, raw strings for backslashes, and punctuation. No new or meaningfully changed docstrings.
Second adds minor rephrases and reformats, like "Gets xyz" → "Get xyz".
Third adds restructures and rephrases. A couple are check-dodging ("Home directory for...") but I disagree with D401 on properties anyway
Relates to #1765
Checklist
make qa
(runsmake quality
andmake test
)