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

quality: Docstring cleanup for pydocstyle prep #1978

Closed

Conversation

half-duplex
Copy link
Member

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

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

Sorry, something went wrong.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@half-duplex half-duplex changed the title quality: Docstring formatting quality: Docstring cleanup for pydocstyle prep Oct 31, 2020

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@half-duplex
Copy link
Member Author

half-duplex commented Oct 31, 2020

To tl;dr IRC:

  • D401's heuristics and its enforcement on properties are somewhat annoying, though slightly improved in v3+
  • Exirel does the most with docs and doesn't want "[this] tool that enforces only compliance, not quality"

Copy link
Contributor

@Exirel Exirel left a 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.
Copy link
Contributor

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.
Copy link
Contributor

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."""
Copy link
Contributor

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:

Suggested change
"""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.
Copy link
Contributor

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."""
Copy link
Contributor

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""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.
Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Devoice users in a room.
"""Devoice users in a channel.

@dgw dgw added this to the 8.0.0 milestone May 10, 2021
@dgw
Copy link
Member

dgw commented Dec 23, 2021

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 make target so it can report any violations in this category without failing the build (as described in PyCQA/flake8#515 (comment)). We can keep an eye on the status as our PRs churn and maybe decide to enforce it later.

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.)

@dgw dgw added Housekeeping Code cleanup, removal of deprecated stuff, etc. Stale Mostly used for PRs that no longer work and need updating before re-review/merge. Declined Requests that will not be implemented for technical or project direction reasons labels Feb 24, 2022
@dgw dgw removed this from the 8.0.0 milestone Feb 24, 2022
@dgw
Copy link
Member

dgw commented Feb 24, 2022

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 make quality step in CI. But more importantly, we can avoid having one monolithic PR that tries to touch everything.

@dgw dgw closed this Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Declined Requests that will not be implemented for technical or project direction reasons Housekeeping Code cleanup, removal of deprecated stuff, etc. Stale Mostly used for PRs that no longer work and need updating before re-review/merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants