Skip to content
This repository was archived by the owner on Dec 16, 2022. It is now read-only.

Conversation

krischer
Copy link
Contributor

It is pretty useful to be able to enforce that docstrings exist. This PR adds a new CLI flag --raise-on-missing-docstrings that, as the name implies, raises errors if docstrings for public functions or methods do not exist.

Support has also been added to the config object and files.

The error codes

  • I002: Missing docstring for public method.
  • I003: Missing docstring for public function.

are inspired by the pydocstyle errors: https://github.com/PyCQA/pydocstyle/blob/cc5a96b356e2f10e8c95f1ca719efa3380237671/src/pydocstyle/violations.py#L171

The I1XX group has already been taken so I used the I0XX group.

I'm not sure if you'd like to guard this behind the --raise-on-missing-docstrings flag as it is currently implemented or just use it by default.

Copy link
Owner

@terrencepreilly terrencepreilly left a comment

Choose a reason for hiding this comment

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

Looks good so far; if you could address the comments I've left, I'll merge it in.

self.function = function
if function.docstring is not None:
try:
try:
Copy link
Owner

Choose a reason for hiding this comment

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

Could you move the try-block back inside of the conditional? The else-branch can't raise a ParserException, so there's no need for it to be included in the try-block.

for variant in ['# noqa: *', '# noqa: S001', '# noqa']:
self.has_no_errors(program.format(variant))

self.config = Configuration(
Copy link
Owner

Choose a reason for hiding this comment

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

Remove this, or comment on its purpose. (It's a little confusing here.)

raise_on_missing_docstrings=False
)

def test_check_for_missing_docstrings(self):
Copy link
Owner

Choose a reason for hiding this comment

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

This could probably be broken up into multiple unit tests. If it's in one unit tests because of the hassle of performing configuration, etc., then you could put it into its own TestCase and have a setUp method. If it's broken up, it will make failing tests easier to diagnose. If we somehow broke the check for missing docstrings on functions but not methods, for example, then the method's name would immediately let us know what is wrong.

# If not docstring is given, and raise on missing is true, and its not
# a private function, add an error.
elif self.config.raise_on_missing_docstrings and not function.name.startswith("_"):
cls = (MissingDocstringForPublicMethod
Copy link
Owner

Choose a reason for hiding this comment

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

This would probably be more understandable if we moved the if-else to the top level, like

if function.is_method:
    self.errors.append(
        ...
    )
else:
    self.errors.append(
        ...
    )

That will make it more maintainable as well. (Say, if we decide we need to pass more information for functions, or if we change the __init__ signature for these twe errors.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants