Repository: pandas-dev/pandas
Pull request: BUG: Ignore versionadded directive when checking for periods at docstring end
Will ignore the `versionadded` directive when checking for `'.'` at the end of descriptions.  

- [x] closes #22405
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Problem statement:
Consider directives when validating docstrings parameters
When running the docstring validation script, errors are mistakenly reported if parameters have a `deprecated`, `versionadded` or `versionchanged` directive.

For example, when running `./scripts/validate_docstrings.py pandas.DataFrame.mode`, the next error is returned by the script:

```
################################################################################
################################## Validation ##################################
################################################################################

Errors found:
	Errors in parameters section
		Parameter "dropna" description should finish with "."
```
But the `dropna` parameter description does finish with a period:

```
dropna : bool, default True
    Don't consider counts of NaN/NaT.

    .. versionadded:: 0.24.0
```

The problem is that the `.. versionadded::` directive is not considered.

The script should ignore the directive when validating, and shouldn't report the error in this case.

Hints from issue discussion:
Now adding `deprecated` and `versionchanged` directives as well.

Commit message: TST: Add multiple directives to tests

Patch to review:
diff --git a/pandas/tests/scripts/test_validate_docstrings.py b/pandas/tests/scripts/test_validate_docstrings.py
index 1d35d5d30bba3..e4f7d776aa713 100644
--- a/pandas/tests/scripts/test_validate_docstrings.py
+++ b/pandas/tests/scripts/test_validate_docstrings.py
@@ -193,6 +193,53 @@ def contains(self, pat, case=True, na=np.nan):
         """
         pass
 
+    def mode(self, axis=0, numeric_only=False, dropna=True):
+        """
+        Get the mode(s) of each element along the axis selected. Adds a row
+        for each mode per label, fills in gaps with nan.
+
+        This test is to ensure that directives don't affect the tests for
+        periods at the end of parameters.
+        Note that there could be multiple values returned for the selected
+        axis (when more than one item share the maximum frequency), which is
+        the reason why a dataframe is returned. If you want to impute missing
+        values with the mode in a dataframe ``df``, you can just do this:
+        ``df.fillna(df.mode().iloc[0])``
+
+        Parameters
+        ----------
+        axis : {0 or 'index', 1 or 'columns'}, default 0
+            Describe axis.
+
+            .. versionchanged:: 0.1.2
+
+        numeric_only : boolean, default False
+            Describes numeric_only.
+
+            .. versionadded:: 0.1.2
+            .. deprecated:: 0.00.0
+
+        dropna : boolean, default True
+            This param tests that the versionadded directive doesn't break the
+            checks for the ending period.
+            Don't consider counts of NaN/NaT.
+
+            .. versionadded:: 0.24.0
+
+        Returns
+        -------
+        modes : DataFrame (sorted)
+
+        Examples
+        --------
+        >>> df = pd.DataFrame({'A': [1, 2, 1, 2, 1, 2, 3]})
+        >>> df.mode()
+           A
+        0  1
+        1  2
+        """
+        pass
+
 
 class BadGenericDocStrings(object):
     """Everything here has a bad docstring
@@ -374,6 +421,31 @@ def no_description_period(self, kind):
            Doesn't end with a dot
         """
 
+    def no_description_period_with_directive(self, kind):
+        """
+        Forgets to add a period, and also includes a directive.
+
+        Parameters
+        ----------
+        kind : str
+           Doesn't end with a dot
+
+           .. versionadded:: 0.00.0
+        """
+
+    def no_description_period_with_directives(self, kind):
+        """
+        Forgets to add a period, and also includes multiple directives.
+
+        Parameters
+        ----------
+        kind : str
+           Doesn't end with a dot
+
+           .. versionchanged:: 0.00.0
+           .. deprecated:: 0.00.0
+        """
+
     def parameter_capitalization(self, kind):
         """
         Forgets to capitalize the description.
@@ -495,7 +567,7 @@ def test_good_class(self):
 
     @pytest.mark.parametrize("func", [
         'plot', 'sample', 'random_letters', 'sample_values', 'head', 'head1',
-        'contains'])
+        'contains', 'mode'])
     def test_good_functions(self, func):
         assert validate_one(self._import_path(   # noqa: F821
             klass='GoodDocStrings', func=func)) == 0
@@ -531,6 +603,8 @@ def test_bad_generic_functions(self, func):
           'Parameter "kind: str" has no type')),
         ('BadParameters', 'no_description_period',
          ('Parameter "kind" description should finish with "."',)),
+        ('BadParameters', 'no_description_period_with_directive',
+         ('Parameter "kind" description should finish with "."',)),
         ('BadParameters', 'parameter_capitalization',
          ('Parameter "kind" description should start with a capital letter',)),
         pytest.param('BadParameters', 'blank_lines', ('No error yet?',),
diff --git a/scripts/validate_docstrings.py b/scripts/validate_docstrings.py
index cdea2d8b83abd..fe3c1fa58e461 100755
--- a/scripts/validate_docstrings.py
+++ b/scripts/validate_docstrings.py
@@ -42,6 +42,7 @@
 
 
 PRIVATE_CLASSES = ['NDFrame', 'IndexOpsMixin']
+DIRECTIVES = ['.. versionadded', '.. versionchanged', '.. deprecated']
 
 
 def _load_obj(obj_name):
@@ -465,7 +466,17 @@ def validate_one(func_name):
                 param_errs.append('Parameter "{}" description '
                                   'should start with a '
                                   'capital letter'.format(param))
-            if doc.parameter_desc(param)[-1] != '.':
+
+            period_check_index = -1
+            for directive in DIRECTIVES:
+                if directive in doc.parameter_desc(param):
+                    # Get index of character before start of directive
+                    index = doc.parameter_desc(param).index(directive) - 1
+                    # If this directive is closest to the description, use it.
+                    if index < period_check_index or period_check_index is -1:
+                        period_check_index = index
+
+            if doc.parameter_desc(param)[period_check_index] != '.':
                 param_errs.append('Parameter "{}" description '
                                   'should finish with "."'.format(param))
     if param_errs:


Code review comments:
diff_hunk: @@ -465,7 +466,17 @@ def validate_one(func_name):
                 param_errs.append('Parameter "{}" description '
                                   'should start with a '
                                   'capital letter'.format(param))
-            if doc.parameter_desc(param)[-1] != '.':
+
+            period_check_index = -1
+            for directive in DIRECTIVES:
+                if directive in doc.parameter_desc(param):
+                    # Get index of character before start of directive
+                    index = doc.parameter_desc(param).index(directive) - 1
+                    # If this directive is closest to the description, use it.
+                    if index < period_check_index or period_check_index is -1:
+                        period_check_index = index
+
+            if doc.parameter_desc(param)[period_check_index] != '.':
path: scripts/validate_docstrings.py
text: @user1:
What do you think about creating a new property in the class `Docstring` that returns the `parameter_desc` but without directives?

I think the code will be much more readable if we have this logic there, and in this part of the code where all validations happen we simply have something like `if doc.parameter_desc_without_directives(param)[-1] != '.':`

@author:
Yes, I agree that would be more useful and readable; I'll make a property.

diff_hunk: @@ -193,6 +193,53 @@ def contains(self, pat, case=True, na=np.nan):
         """
         pass
 
+    def mode(self, axis=0, numeric_only=False, dropna=True):
path: pandas/tests/scripts/test_validate_docstrings.py
text: @user1:
I think a smaller, more directed example would be preferable here (it doesn't need to match the original docstring and probably won't over time anyway). Can you strip this down to just what's important?

@author:
Sure thing, sounds good.

diff_hunk: @@ -42,6 +42,7 @@
 
 
 PRIVATE_CLASSES = ['NDFrame', 'IndexOpsMixin']
+DIRECTIVES = ['.. versionadded', '.. versionchanged', '.. deprecated']
path: scripts/validate_docstrings.py
text: @user1:
afaik, all sphinx directives start with `.. ` so I think it's better to have just the names here.