Recommend Google Style for DocStrings (#88)

* Recommend sphinx as the docstring format

* Update docs/Coding-Conventions.md

Co-authored-by: mshafer-NI <[email protected]>

* Change the convention to be Google style

* Update docs/Coding-Conventions.md

Co-authored-by: Ryan Zoeller <[email protected]>

* Allow any convention style

* add org-check to PR notify

* touch conventions file to prompt PR checks

* touch again

* set docstring convention to Google

* add some debug output

* document D412

* rname vars and re-work to bettter show what the error is

* switch to "all" and suprress the codes that we see that don't fit the "suggest Google"

* format

* revert most of the docstring changes

* remove blank lines before closing quotes

* fix branch name in notify action

* add note about using sphinx.ext.napolean to handle docs

* Revert "revert most of the docstring changes"

This reverts commit c09c931.

* more docstrings to active form

* add D406 to excluded rules

* s/sell/sells/g

* add all the codes pydocstyle would ignore if we were explicitly stating Google style

* don't need that twice

Co-authored-by: Joshua Cannon <[email protected]>
Co-authored-by: Joshua Cannon <[email protected]>
Co-authored-by: Ryan Zoeller <[email protected]>

Author: 4 people

docs/Coding-Conventions.md

@@ -982,6 +982,12 @@ def go_shopping():
 
 ℹ️ You can document a package by documenting the module docstring of the package directory's `__init__.py`
 
+### Which docstring format should I follow?
+
+We recommend (and internally use) the [Google docstring format](https://google.github.io/styleguide/pyguide.html#383-functions-and-methods) but you can choose any format so long as you are consistent.
+
+**Note**: Through the use of the [Sphinx napoleon](https://www.sphinx-doc.org/en/master/usage/extensions/napoleon.html#getting-started) extension, Sphinx docs generation can interpret [Google style docstrings](https://google.github.io/styleguide/pyguide.html#383-functions-and-methods).
+
 ### [D.1.2] ✔️ **DO** List exported modules and subpackages in a package's docstring
 
 > 🐍 This rule stems from [PEP 257](https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings)
@@ -1070,7 +1076,7 @@ The summary line should be on the same line as the opening quotes.
 ```python
 # Bad - will produce D205
 def sell(type_):
-    """Sell the specified type of cheese.
+    """Sells the specified type of cheese.
     Will throw an OutOfStockException if the specified type of cheese is out of stock.
     """
 ```
@@ -1079,7 +1085,7 @@ def sell(type_):
 # Bad - will produce D212
 def sell(type_):
     """
-    Sell the specified type of cheese.
+    Sells the specified type of cheese.
 
     Will throw an OutOfStockException if the specified type of cheese is out of stock.
     """
@@ -1088,7 +1094,7 @@ def sell(type_):
 ```python
 # Good
 def sell(type_):
-    """Sell the specified type of cheese.
+    """Sells the specified type of cheese.
 
     Will throw an OutOfStockException if the specified type of cheese is out of stock.
     """
@@ -1100,7 +1106,7 @@ class CheeseShop(object):
     """Finest cheese shop in the district, offering a wide variety of cheeses."""
 
     def sell(self, type_):
-        """Sell the specified type of cheese."""
+        """Sells the specified type of cheese."""
 ```
 
 ### [D.1.11] ✔️ **DO** Put closing `"""` on its own line for multiline docstrings 💻
@@ -1117,7 +1123,7 @@ class CheeseShop(object):
     Cheeses are sold first-come-first-served, and can run out of stock rather quickly."""
 
     def sell(self, type_):
-        """Sell the specified type of cheese.
+        """Sells the specified type of cheese.
 
         Will throw an OutOfStockException if the specified type of cheese is out of stock."""
 ```
@@ -1131,7 +1137,7 @@ class CheeseShop(object):
     """
 
     def sell(self, type_):
-        """Sell the specified type of cheese.
+        """Sells the specified type of cheese.
 
         Will throw an OutOfStockException if the specified type of cheese is out of stock.
         """
@@ -1157,7 +1163,7 @@ class CheeseShop(object):
 class CheeseShop(object):
     def sell(self, type_):
 
-        """Sell the specified type of cheese."""
+        """Sells the specified type of cheese."""
 ```
 
 ```python
@@ -1166,7 +1172,7 @@ class CheeseShop(object):
     """Finest cheese shop in the district, offering a wide variety of cheeses."""
 
     def sell(self, type_):
-        """Sell the specified type of cheese."""
+        """Sells the specified type of cheese."""
 ```
 
 ### [D.1.13] ❌ **DO NOT** Put a blank line after a one line function docstring 💻
@@ -1178,18 +1184,55 @@ class CheeseShop(object):
 ```python
 # Bad
 def sell(self, type_):
-    """Sell the specified type of cheese."""
+    """Sells the specified type of cheese."""
 
     self._do_transaction(type_)
 ```
 
 ```python
 # Good
 def sell(self, type_):
-    """Sell the specified type of cheese."""
+    """Sells the specified type of cheese."""
     self._do_transaction(type_)
 ```
 
+
+### [D.1.14] ❌ **DO NOT** Put a blank line after section headers 💻
+
+> 💻 This rule is enforced by error code D412
+
+```python
+# Bad - will produce D412
+class CheeseShop(object):
+    def sell(self, type_):
+        """Sells the specified type of cheese.
+
+        Args:
+
+            type_: the desired type
+        """
+        self._do_transaction(type_)
+```
+
+```python
+# Good
+class CheeseShop(object):
+    def sell(self, type_: str):
+        """Sells the specified type of cheese.
+
+        Args:
+            type_: the desired cheese type
+        """
+        self._do_transaction(type_)
+```
+
+```python
+# Best
+class CheeseShop(object):
+    def sell(self, type_: str):
+        """Sells the specified type of cheese."""
+        self._do_transaction(type_)
+```
 ---
 
 # [C] Comments

ni_python_styleguide/_acknowledge_existing_errors/__init__.py

@@ -35,7 +35,7 @@ def _filter_suppresion_from_line(line: str):
 def acknowledge_lint_errors(
     exclude, app_import_names, extend_ignore, file_or_dir, *_, aggressive=False
 ):
-    """Add a "noqa" comment for each of existing errors (unless excluded).
+    """Adds a "noqa" comment for each of existing errors (unless excluded).
 
     Excluded error (reason):
     BLK100 - run black
@@ -94,7 +94,7 @@ def acknowledge_lint_errors(
 
 
 def remove_auto_suppressions_from_file(file: pathlib.Path):
-    """Remove auto-suppressions from file."""
+    """Removes auto-suppressions from file."""
     lines = file.read_text(encoding=_utils.DEFAULT_ENCODING).splitlines()
     stripped_lines = [_filter_suppresion_from_line(line) for line in lines]
     file.write_text("\n".join(stripped_lines) + "\n", encoding=_utils.DEFAULT_ENCODING)

ni_python_styleguide/_cli.py

@@ -38,7 +38,7 @@ def _read_pyproject_toml(ctx, param, value):
 
 
 def _get_application_import_names(pyproject):
-    """Return the application package name the config."""
+    """Returns the application package name the config."""
     # Otherwise override with what was specified
     app_name = (
         pyproject.get("tool", {})
@@ -57,7 +57,7 @@ class ConfigGroup(click.Group):
     """click.Group subclass which allows for a config option to load options from."""
 
     def __init__(self, *args, **kwargs):
-        """Construct the click.Group with the config option."""
+        """Constructs the click.Group with the config option."""
         kwargs["params"].append(
             click.Option(
                 ["--config"],

ni_python_styleguide/_fix.py

@@ -15,7 +15,7 @@
 
 
 def _split_imports_line(lines: str, *_, **__):
-    r"""Split multi-import lines to multiple lines.
+    r"""Splits multi-import lines to multiple lines.
 
     >>> _split_imports_line("import os, collections\n")
     'import os\nimport collections\n'

ni_python_styleguide/_format.py

@@ -8,7 +8,7 @@
 
 
 def format(file_or_dir, *additional_formatter_args):
-    """Format the specified file or directory using the builtin config."""
+    """Formats the specified file or directory using the builtin config."""
     try:
         black.main(
             [
@@ -25,7 +25,7 @@ def format(file_or_dir, *additional_formatter_args):
 
 
 def format_check(file_or_dir):
-    """Perform format and return True if changes were made (False otherwise)."""
+    """Formats and returns True if changes were made (False otherwise)."""
     capture = StringIO()
     with contextlib.redirect_stderr(capture):
         format(file_or_dir)

ni_python_styleguide/_utils/code_analysis.py

@@ -4,7 +4,7 @@
 
 
 def find_import_region(file: pathlib.Path) -> Tuple[int, int]:
-    """Return the index of the first last import line that precedes any other code.
+    """Returns the index of the first last import line that precedes any other code.
 
     Note: will not handle try/except imports
 

ni_python_styleguide/_utils/lint.py

@@ -26,7 +26,7 @@ def get_errors_to_process(exclude, app_import_names, extend_ignore, file_or_dir,
 
 def parse(line):
     r"""
-    Parse line into :class:`LintError`.
+    Parses line into :class:`LintError`.
 
     >>> parse(r'source\arfile.py:55:16: BLK100 Black would make changes.')
     LintError(file='source\\arfile.py', line=55, column=16, code='BLK100', explanation='Black would make changes.')
@@ -63,7 +63,7 @@ def _to_lint_error(file: str, line: str, column: str, code: str, explanation: st
         )
 
     def parse(self, line):
-        """Parse `line` and return a :class:`LintError`.
+        """Parses `line` and return a :class:`LintError`.
 
         :param line: the line to parse
         :return: lint error as metada object

ni_python_styleguide/_utils/string_helpers.py

@@ -23,11 +23,11 @@ def __init__(self, error_file: Optional[str] = None, *_, lines: Optional[List[st
 
     @property
     def values(self):
-        """Return the values for the file."""
+        """The values for the file."""
         return self._values
 
     def in_multiline_string(self, lineno):
-        """Check if lineno is in a multiline string."""
+        """Checks if lineno is in a multiline string."""
         return self._values[lineno - 1]  # 0 indexed, but we number files 1 indexed
 
     @staticmethod

ni_python_styleguide/config.ini

@@ -65,6 +65,15 @@ ignore =
     F812
 
     # flake8-docstrings
+    # Ignoring the requirement on imperative mood, as not all conventions agree on this
+    # and ultimately is up to the developer. If, in the future, the mood was enforced based on
+    # convention choice we can consider removing this supression as that would make documentation
+    # consistent within a standard (but maybe not a codebase)
+    # Developers are welcome to turn back on this error in their specific projects.
+    # The same goes for the "descriptive"-mood error, if one was to be added to pydocstyle.
+    D401
+    # Conflicts with recommending Google style DocStrings
+    D406  # Section name should end with a newline -> forces a newline at end of DocString
     # Ignoring things enforced by black
     D200  # One-line docstring should fit on one line with quotes
     D206  # Docstring should be indented with spaces, not tabs
@@ -76,7 +85,19 @@ ignore =
     D203  # 1 blank line required before class docstring
     D213  # Multi-line docstring summary should start at the second line
     D400  # First line should end with a period
-
+    # Recommending Google, so these don't apply - http://www.pydocstyle.org/en/stable/error_codes.html#:~:text=The%20google%20convention%20added%20in%20v4.0.0
+    D203
+    D204
+    D213
+    D215
+    D400
+    D401
+    D404
+    D406
+    D407
+    D408
+    D409
+    D413
     # flake8-import-order
     I101  #  The names in your from import are in the wrong order. (Enforced by E401)
 

tests/conftest.py

@@ -9,14 +9,14 @@
 
 
 def pytest_collection_modifyitems(items):
-    """Ignore deprecation warnings in all tests."""
+    """Ignores deprecation warnings in all tests."""
     for item in items:
         item.add_marker(pytest.mark.filterwarnings("ignore::DeprecationWarning"))
 
 
 @pytest.fixture
 def styleguide(monkeypatch, cli_runner):
-    """Fixture which runs the styleguide when executed.
+    """Runs the styleguide when executed.
 
     Args passed to the function are equivalent to CLI args,
     and are automatically stringified.
@@ -55,7 +55,7 @@ def runner(*, base_args=[], command="", command_args=[]):
 
 @pytest.fixture
 def chdir():
-    """Fixture which changes the current working directory when executed."""
+    """Changes the current working directory when executed."""
     cwd = os.getcwd()
     yield os.chdir
     os.chdir(cwd)