BUG: Ignore versionadded directive when checking for periods at docstring end #22423

bengineer19 · 2018-08-19T18:52:41Z

Will ignore the versionadded directive when checking for '.' at the end of descriptions.

closes Consider directives when validating docstrings parameters #22405
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

…ring end

WillAyd · 2018-08-19T18:54:11Z

Can you add a test case for this? We also have a versionchanged directive which we should account for here

TomAugspurger · 2018-08-19T19:06:58Z

And the deprecated directive as well probably.

codecov · 2018-08-19T20:28:23Z

Codecov Report

Merging #22423 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #22423      +/-   ##
==========================================
- Coverage   92.05%   92.04%   -0.01%     
==========================================
  Files         169      169              
  Lines       50714    50740      +26     
==========================================
+ Hits        46684    46705      +21     
- Misses       4030     4035       +5

Flag	Coverage Δ
#multiple	`90.45% <ø> (-0.01%)`	⬇️
#single	`42.24% <ø> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/util/_depr_module.py	`65.11% <0%> (-2.33%)`	⬇️
pandas/core/reshape/pivot.py	`96.55% <0%> (-0.63%)`	⬇️
pandas/core/arrays/integer.py	`94.55% <0%> (-0.12%)`	⬇️
pandas/util/testing.py	`85.75% <0%> (-0.11%)`	⬇️
pandas/core/reshape/merge.py	`94.15% <0%> (-0.01%)`	⬇️
pandas/core/groupby/grouper.py	`98.16% <0%> (-0.01%)`	⬇️
pandas/core/generic.py	`96.44% <0%> (-0.01%)`	⬇️
pandas/core/frame.py	`97.24% <0%> (-0.01%)`	⬇️
pandas/core/series.py	`93.73% <0%> (ø)`	⬆️
pandas/io/parsers.py	`95.48% <0%> (ø)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 140c7bb...3039d78. Read the comment docs.

datapythonista · 2018-08-20T13:14:54Z

scripts/validate_docstrings.py

+                    if index < period_check_index or period_check_index is -1:
+                        period_check_index = index
+
+            if doc.parameter_desc(param)[period_check_index] != '.':


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] != '.':

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

datapythonista · 2018-08-20T13:16:05Z

scripts/validate_docstrings.py

@@ -42,6 +42,7 @@


 PRIVATE_CLASSES = ['NDFrame', 'IndexOpsMixin']
+DIRECTIVES = ['.. versionadded', '.. versionchanged', '.. deprecated']


afaik, all sphinx directives start with .. so I think it's better to have just the names here.

WillAyd · 2018-08-20T17:23:49Z

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


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?

Sure thing, sounds good.

WillAyd · 2018-08-20T20:35:07Z

scripts/validate_docstrings.py

@@ -236,6 +237,15 @@ def parameter_type(self, param):
    def parameter_desc(self, param):
        return self.doc_parameters[param][1]

+    def parameter_desc_without_directives(self, param):


What happens if you just use this logic in parameter_desc instead of as a separate method? I can't think of a case where we want the directives to be considered part of the description, and the way you've coded this works well for the missing period but may not be generalizable to other issues that could come up

That's a good point. I'd probably keep a raw_parameter_desc with what parameter_desc has now, as in the future I'd probably like to have the values of the directives (for example to find the deprecated parameters we need to delete in the next version).

WillAyd · 2018-08-21T21:26:33Z

scripts/validate_docstrings.py

@@ -233,9 +234,19 @@ def correct_parameters(self):
    def parameter_type(self, param):
        return self.doc_parameters[param][0]

-    def parameter_desc(self, param):
+    def raw_parameter_desc(self, param):


@datapythonista I know you wanted this but I would prefer not to include unless it serves a purpose. May even be better served as a keyword argument going forward instead of a dedicated method so would rather not go down this path until needed.

Outside of that I think this change looks good

Sure. We can just have parameter_desc for now, and see when we start using the directives what makes more sense.

datapythonista

Looks good, but I was checking some docstrings, and there are couple of cases we didn't consider. Can you take care of them?

datapythonista · 2018-08-22T08:54:36Z

scripts/validate_docstrings.py

@@ -233,9 +234,19 @@ def correct_parameters(self):
    def parameter_type(self, param):
        return self.doc_parameters[param][0]

-    def parameter_desc(self, param):
+    def raw_parameter_desc(self, param):


Sure. We can just have parameter_desc for now, and see when we start using the directives what makes more sense.

datapythonista · 2018-08-22T09:09:32Z

pandas/tests/scripts/test_validate_docstrings.py

+            Sentence ending in period, followed by multiple directives.
+
+            .. versionadded:: 0.1.2
+            .. deprecated:: 0.00.0


I'm not sure about versionadded and versionchanged, but deprecated can have a description after if, for example:

.. deprecated:: 0.21.0 Use :func:`pandas.read_csv` instead.

And it can be even multiline. Do you mind adding a test for that? I'm not sure if this is working with the current implementation.

Also, if you check the convert_datetime64 of to_records, there are cases where the directives come before the description. I'm happy if we consider only valid having them in one place (before or after the description). But, can we make the script generate a descriptive error for it? I guess with the current implementation we'll report that the parameter has no description.

I've added a test case for multi-line descriptions.
Directive positioning is a bit more tricky. Enforcing them to be in one place would help, but the problem comes when trying to determine if text after the directive is directive description, or just generic parameter description. We need to make this distinction in order to produce a nice error message.
This is made harder by the fact that we're currently working with doc_parameters, which smooshes the whole description into one single-line string.

I think enforcement after description is fine. I think @datapythonista is correct in that it will generate an error, albeit with the wrong message. If we wanted to clean that up I'd suggest a separate PR, though @datapythonista I'll leave that decision up to you

WillAyd

This looks good to me

datapythonista

This will fix most cases, happy to merge on green, and take care of other cases in separate PRs. Thanks @bengineer19

…port on missing period (pandas-dev#22423)

BUG: Ignore versionadded directive when checking for periods at docst…

ac1ca24

…ring end

bengineer19 added 3 commits August 19, 2018 20:36

TST: Added test cases for versionadded directive

b06cdbf

BUG: Refactor to ignore versionchanged and deprecated directives.

25382b9

TST: Add multiple directives to tests

cd861fe

gfyoung added the Docs label Aug 19, 2018

datapythonista reviewed Aug 20, 2018

View reviewed changes

WillAyd requested changes Aug 20, 2018

View reviewed changes

bengineer19 added 2 commits August 20, 2018 19:42

CLN: Add property for parameter descriptions excluding directives

cb4bcc3

TST: Update test cases for ignoring directives to be more concise.

748b167

WillAyd reviewed Aug 20, 2018

View reviewed changes

CLN: Change parameter_desc to strip out directives

37da2cc

WillAyd reviewed Aug 21, 2018

View reviewed changes

datapythonista requested changes Aug 22, 2018

View reviewed changes

datapythonista mentioned this pull request Aug 22, 2018

Changes to validate_docstring script to be able to check all docstrings at once #22408

Merged

4 tasks

bengineer19 added 2 commits August 22, 2018 18:28

CLN: Remove raw_parameter_desc as it currently serves no purpose

ccfa4d6

TST: Test directive descriptions

3039d78

WillAyd approved these changes Aug 22, 2018

View reviewed changes

datapythonista approved these changes Aug 22, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Aug 23, 2018

datapythonista merged commit 0f656f7 into pandas-dev:master Aug 23, 2018

bengineer19 deleted the validate_docstrings_versionadded branch August 23, 2018 17:57

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

DOC: Docstring validation ignoring directives in parameters to not re…

b7db14b

…port on missing period (pandas-dev#22423)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Ignore versionadded directive when checking for periods at docstring end #22423

BUG: Ignore versionadded directive when checking for periods at docstring end #22423

bengineer19 commented Aug 19, 2018

WillAyd commented Aug 19, 2018

TomAugspurger commented Aug 19, 2018

codecov bot commented Aug 19, 2018 •

edited

Loading

datapythonista Aug 20, 2018

bengineer19 Aug 20, 2018

datapythonista Aug 20, 2018

WillAyd Aug 20, 2018

bengineer19 Aug 20, 2018

WillAyd Aug 20, 2018

datapythonista Aug 21, 2018

WillAyd Aug 21, 2018

datapythonista Aug 22, 2018

datapythonista left a comment

datapythonista Aug 22, 2018

datapythonista Aug 22, 2018

datapythonista Aug 22, 2018

bengineer19 Aug 22, 2018

WillAyd Aug 22, 2018

WillAyd left a comment

datapythonista left a comment

		@@ -42,6 +42,7 @@


		PRIVATE_CLASSES = ['NDFrame', 'IndexOpsMixin']
		DIRECTIVES = ['.. versionadded', '.. versionchanged', '.. deprecated']

BUG: Ignore versionadded directive when checking for periods at docstring end #22423

BUG: Ignore versionadded directive when checking for periods at docstring end #22423

Conversation

bengineer19 commented Aug 19, 2018

WillAyd commented Aug 19, 2018

TomAugspurger commented Aug 19, 2018

codecov bot commented Aug 19, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datapythonista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd left a comment

Choose a reason for hiding this comment

datapythonista left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 19, 2018 •

edited

Loading