Introduce convention class #589

Mr-Pepe · 2022-04-14T20:23:51Z

This PR implements the first step towards using the specified convention to assume the style of docstrings and not rely on figuring out whether a docstring uses NumPy or Google style (#459).

This PR introduces a new class Convention that holds the error codes to check. It also keeps the name of the convention to use. I imagine that an instance of this class should be passed to the ConventionChecker. Convention-specific checks like _check_numpy_sections and _check_google_sections can then be executed based on the specified convention. This would eventually solve #459.

I added tests for the code I added and all existing tests still pass.

Mr-Pepe · 2022-05-15T20:17:51Z

Is there any interest in moving this forward?

benji-york

I'm not a pydocstyle maintainer, so this review doesn't mean much, but this looks like a nice improvement to the code. It mostly just ties together ideas that were pre-existing into concrete representations.

I made a couple of small comments inline.

benji-york · 2022-06-18T09:04:32Z

src/pydocstyle/conventions.py

+        'D409',
+        'D413',
+    },
+}


I'm wondering if there is a common subset of the errors being removed from each of the conventions that makes sense to name. E.g., something along the lines of "very strict rules".

I actually don't love the "all_errors - {subset}" logic, but not just because of code duplication. Any time anyone adds a new convention, we have to add it to this list. Personally, it makes more sense to me to make these additive instead of subtractive. But let's save this for another issue/PR since this PR doesn't change anything, just moves it around.

benji-york · 2022-06-18T09:06:19Z

src/pydocstyle/conventions.py

+        The convention has two purposes. First, it holds the error codes to be
+        checked. Second, it defines how to treat docstrings, eliminating the
+        need for extra logic to determine whether a docstring is a NumPy or
+        Google style docstring.


The second part seems especially useful to me. Going very far down the road to DWIM ("do what I mean") gets pretty painful. Users end up having to trick the tool into doing what they want instead of just specifying it as this approach allows.

samj1912 · 2023-01-08T17:53:46Z

@adamjstewart / @Pierre-Sassoulas would you like to review this?

Pierre-Sassoulas

Isn't the numpy/google conventions also following pep 257 ?

requirements/runtime.txt

adamjstewart · 2023-01-08T18:56:15Z

On vacation this week but will review next week.

adamjstewart

Alright, finally got a chance to test this. Using this branch, I was able to find and fix several new bugs in TorchGeo's docs: microsoft/torchgeo#1011. I can confirm that this PR fixes #459!

Note that there were a couple of odd things I noticed. First was the output of pydocstyle:

$ pydocstyle .
./torchgeo/datasets/gid15.py:247 in public method `plot`:
        D417: Missing argument descriptions in the docstring (argument(s) suptitle are missing descriptions in 'plot' docstring)
./torchgeo/datasets/cyclone.py:226 in public method `plot`:
        D417: Missing argument descriptions in the docstring (argument(s) show_titles, suptitle are missing descriptions in 'plot' docstring)
./torchgeo/datamodules/vaihingen.py:39 in public method `__init__`:
        D417: Missing argument descriptions in the docstring (argument(s) num_patches_per_tile, num_workers, patch_size, val_split_pct are missing descriptions in '__init__' docstring)
./torchgeo/datamodules/deepglobelandcover.py:37 in public method `__init__`:
        D417: Missing argument descriptions in the docstring (argument(s) num_patches_per_tile, num_workers, patch_size, val_split_pct are missing descriptions in '__init__' docstring)
./torchgeo/datamodules/potsdam.py:39 in public method `__init__`:
        D417: Missing argument descriptions in the docstring (argument(s) num_patches_per_tile, num_workers, patch_size, val_split_pct are missing descriptions in '__init__' docstring)
./torchgeo/models/rcf.py:36 in public method `__init__`:
        D417: Missing argument descriptions in the docstring (argument(s) bias, features, kernel_size, seed are missing descriptions in '__init__' docstring)
./torchgeo/samplers/single.py:76 in public method `__init__`:
        D417: Missing argument descriptions in the docstring (argument(s) length, roi, size, units are missing descriptions in '__init__' docstring)
./torchgeo/samplers/single.py:184 in public method `__init__`:
        D417: Missing argument descriptions in the docstring (argument(s) roi, size, stride, units are missing descriptions in '__init__' docstring)
./torchgeo/samplers/single.py:287 in public method `__init__`:
        D417: Missing argument descriptions in the docstring (argument(s) roi, shuffle are missing descriptions in '__init__' docstring)
./torchgeo/samplers/batch.py:74 in public method `__init__`:
        D417: Missing argument descriptions in the docstring (argument(s) batch_size, length, roi, size, units are missing descriptions in '__init__' docstring)

None of these functions are missing any argument descriptions, the problems lay elsewhere. So the error messages don't seem correct.

The other odd thing I noticed was that pydocstyle complains if you put Sphinx directives like versionadded after Args/Returns. I don't believe Google's style guide precludes this, so I'm surprised to see pydocstyle complain.

Both of these issues feel like bugs in pydocstyle, not bugs in this PR. I think we should first push forward with this PR and I can open issues for the above bugs at a later date.

src/pydocstyle/conventions.py

Mr-Pepe · 2023-01-15T14:16:44Z

Rebased on top of master and addressed feedback

adamjstewart

I think something went wrong with the rebase, this branch no longer catches the mistakes found previously in microsoft/torchgeo#1011 and no longer fixes #459.

src/pydocstyle/conventions.py

adamjstewart · 2023-01-15T17:51:18Z

We should add a test case for #459. All of our tests are passing but #459 is still broken.

Mr-Pepe · 2023-01-15T19:39:07Z

I'm actually surprised that this PR fixed #459 for you because it is not supposed to, as outlined in the PR description. I did not touch anything regarding how docstrings are treated or how checks are executed so there must have been something else going on during your tests. This PR only formalizes the concept of conventions a bit.

adamjstewart · 2023-01-15T20:16:38Z

Gotcha, yeah I have no idea why things were working before the rebase, maybe because the branch was old? I'm fine with formalizing the concept and then fixing #459 in a separate PR. Thanks for your work on this!

adamjstewart

Not sure how well I'll be able to review this since I'm not that familiar with the internals. I guess I'm not sure why this PR is needed as a precursor to fix #459, which is what I really care about.

adamjstewart · 2023-01-15T20:45:22Z

src/pydocstyle/conventions.py

+}
+
+
+class Convention:


What's the advantage of using a class instead of just using an Enum and keeping the AttrDict?

My idea was to eventually have the convention class itself check the code and select the appropriate checker function. You can probably also just throw the name of the convention into the convention checker and that's it.

adamjstewart · 2023-01-15T20:53:00Z

src/pydocstyle/conventions.py

+    def add_error_codes(self, error_codes: Set[str]) -> None:
+        """Add additional error codes to the convention.
+
+        Args:
+            error_codes: The error codes to also check.
+        """
+        self.error_codes = self.error_codes.union(error_codes)
+
+    def remove_error_codes(self, error_codes: Set[str]) -> None:


If you instead define __add__/__or__ and __sub__, you can run:

convention = Convention() convention += {"D103"} convention -= {"D101", "D102"}

Yes that's possible, but at least to me it makes the whole thing less obvious, without offering a real benefit.

benji-york approved these changes Jun 18, 2022

View reviewed changes

adamjstewart mentioned this pull request Jan 2, 2023

Call for maintainers #575

Closed

Pierre-Sassoulas reviewed Jan 8, 2023

View reviewed changes

requirements/runtime.txt Outdated Show resolved Hide resolved

adamjstewart mentioned this pull request Jan 12, 2023

Fix mistakes caught by pydocstyle convention PR microsoft/torchgeo#1011

Merged

adamjstewart reviewed Jan 12, 2023

View reviewed changes

src/pydocstyle/conventions.py Outdated Show resolved Hide resolved

adamjstewart reviewed Jan 12, 2023

View reviewed changes

src/pydocstyle/conventions.py Outdated Show resolved Hide resolved

adamjstewart reviewed Jan 12, 2023

View reviewed changes

src/pydocstyle/conventions.py Outdated Show resolved Hide resolved

Mr-Pepe added 4 commits January 14, 2023 11:01

Add Convention class

801fc92

Use convention class to retrieve error codes

6d3a59d

Fix Literal typing for Python 3.7

9a54a2a

Address feedback from PR review

8e9ef40

Mr-Pepe force-pushed the introduce-convention-class branch from 0ba3606 to 8e9ef40 Compare January 15, 2023 14:08

adamjstewart reviewed Jan 15, 2023

View reviewed changes

src/pydocstyle/conventions.py Outdated Show resolved Hide resolved

Mr-Pepe added 2 commits January 15, 2023 20:18

Remove unnecessary import

0a393a2

Fix docstrings

0c9ba5b

adamjstewart reviewed Jan 15, 2023

View reviewed changes

sigmavirus24 closed this Aug 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce convention class #589

Introduce convention class #589

Mr-Pepe commented Apr 14, 2022

Mr-Pepe commented May 15, 2022

benji-york left a comment

benji-york Jun 18, 2022

adamjstewart Jan 12, 2023

benji-york Jun 18, 2022

samj1912 commented Jan 8, 2023

Pierre-Sassoulas left a comment

adamjstewart commented Jan 8, 2023

adamjstewart left a comment

Mr-Pepe commented Jan 15, 2023

adamjstewart left a comment •

edited

adamjstewart commented Jan 15, 2023

Mr-Pepe commented Jan 15, 2023

adamjstewart commented Jan 15, 2023

adamjstewart left a comment

adamjstewart Jan 15, 2023

Mr-Pepe Jan 22, 2023

adamjstewart Jan 15, 2023

Mr-Pepe Jan 22, 2023

Introduce convention class #589

Introduce convention class #589

Conversation

Mr-Pepe commented Apr 14, 2022

Mr-Pepe commented May 15, 2022

benji-york left a comment

Choose a reason for hiding this comment

benji-york Jun 18, 2022

Choose a reason for hiding this comment

adamjstewart Jan 12, 2023

Choose a reason for hiding this comment

benji-york Jun 18, 2022

Choose a reason for hiding this comment

samj1912 commented Jan 8, 2023

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

adamjstewart commented Jan 8, 2023

adamjstewart left a comment

Choose a reason for hiding this comment

Mr-Pepe commented Jan 15, 2023

adamjstewart left a comment • edited

Choose a reason for hiding this comment

adamjstewart commented Jan 15, 2023

Mr-Pepe commented Jan 15, 2023

adamjstewart commented Jan 15, 2023

adamjstewart left a comment

Choose a reason for hiding this comment

adamjstewart Jan 15, 2023

Choose a reason for hiding this comment

Mr-Pepe Jan 22, 2023

Choose a reason for hiding this comment

adamjstewart Jan 15, 2023

Choose a reason for hiding this comment

Mr-Pepe Jan 22, 2023

Choose a reason for hiding this comment

adamjstewart left a comment •

edited