DOC: Add ignore-deprecate argument to validate_docstrings.py #23650

charlesdong1991 · 2018-11-12T20:22:35Z

closes CI: Add --ignore-deprecated to validate_docstrings.py #23631
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2018-11-12T20:22:38Z

Hello @charlesdong1991! Thanks for updating the PR.

There are no PEP8 issues in the file scripts/tests/test_validate_docstrings.py !
There are no PEP8 issues in the file scripts/validate_docstrings.py !

Comment last updated on November 13, 2018 at 12:55 Hours UTC

datapythonista · 2018-11-12T20:51:47Z

Thanks for the PR. Sorry the issue wasn't clear. Let me explain clear better...

validate_docstrings.py can be called for a single method, or for all them. If called for a single method, there is nothing to skip or do, as the most reasonable thing is to show all the errors for the requested docstring.

This is useful only when validating all the docstrings. That, instead of validating 100% of them, we can validate only the ones not deprecated (we don't care that much about errors in the docstrings of deprecated methods, as we are going to delete them in few weeks).

I think you did something different here, if I'm not wrong.

Btw, you should replace in the pull request description the #xxxx by the issue number you're fixing. Then, for every element in the checklist that applies, you should replace [ ] by [X], so we know which items in the list you did.

Please let me know if you need additional info.

datapythonista · 2018-11-12T20:53:53Z

scripts/validate_docstrings.py

@@ -838,8 +842,16 @@ def header(title, width=80, char='#'):
                           'list of error codes to validate. By default it '
                           'validates all errors (ignored when validating '
                           'a single docstring)')
+    argparser.add_argument('--deprecated', default=False, type=bool,


The argument name should probably be --ignore-deprecated, and just deprecated is not very clear

datapythonista

That looks great now, sorry for the bad communication initially. Just added couple of comments about argparse.

datapythonista · 2018-11-12T21:58:36Z

scripts/validate_docstrings.py

@@ -838,8 +842,16 @@ def header(title, width=80, char='#'):
                           'list of error codes to validate. By default it '
                           'validates all errors (ignored when validating '
                           'a single docstring)')
+    argparser.add_argument('--ignore_deprecated', default=False, type=bool,


Not a big difference, but I think in this case it's better to use action='store_true'. I think with type=bool we can have unexpected behavior like --ignore-deprecated=false meaning actually true.

yes, i also feel confused... because set it to true actually means we won't store it...
changed...

datapythonista · 2018-11-12T21:59:50Z

scripts/validate_docstrings.py

+                           'the function or method will be deprecated '
+                           'in later version. Default is True, means if the '
+                           'function will be deprecated, the docstring will '
+                           'not be displayed in the output of script')


I think we can now simplify this description to something like if this flag is set, deprecated objects are ignored when validating all docstrings

nice one! the description looks much clear now!! thx!!!

datapythonista

lgtm, nice PR @charlesdong1991, thanks for the work on this

charlesdong1991 · 2018-11-12T22:47:40Z

thank you @datapythonista excited to learn from you!!

charlesdong1991 · 2018-11-13T20:24:31Z

HI, @datapythonista sorry to bother, i have struggle at this point for a while and tried quite many times to complete the pytest, however, seems that this deprecated is not working... https://travis-ci.org/charlesdong1991/pandas/jobs/454651182
as far as i understand, since the first one's deprecated is True, so error messages of this one will be ignored, thus exit_status should be 2 instead of 3... can you take a look and give me some hints? thanks a lot!

datapythonista · 2018-11-13T21:07:14Z

If I remember correctly, the exit status is the number of errors, not the number of docstrings failing. So, it should be 3, as one docstring generates 2 errors, the other 1, and the other is ignored because it's deprecated, so it will generate 0.

Does that make sense?

charlesdong1991 · 2018-11-13T21:14:37Z

Hello, @datapythonista thank you very much for your quick response! Yes, i think exit status is the number of errors, however, in my case, i specified the error parameter to "ER01" which means it will only count the errors that are ER01, so in this case, only two will be chosen, because although the first docstring contains ER01, it's deprecated, so should not be counted. please correct me if i understand incorrectly.

datapythonista · 2018-11-13T21:24:02Z

I think the problem with the test is that the filtering is happening inside of validate_all, but you are actually testing main assuming that validate_all returns what's being mocked, which does not filter the deprecated docstrings. Does that make sense?

charlesdong1991 · 2018-11-13T21:32:08Z

@datapythonista agree! i am thinking about the same, i also doubt if i should test the main... but i haven't found any test class or functions for validate_all? any suggestions on which is the best way to test? shall i create mock data as input and set deprecated to True, and then count the length of dictionary of outcome to see if the length is reduced accordingly to the deprecation?

datapythonista · 2018-11-13T21:50:51Z

I do think we have tests where we mock validate_one and test validate_all. Can you take a look? If we don't, we should create them anyway, but I think you should have them implemented so you can base the new one on those.

charlesdong1991 · 2018-11-14T13:30:31Z

Hi, @datapythonista sorry to bother you again, i made new changes on pytest, and yesterday it seemed work and passed tests... however, today when i am trying to complete PR, i see some other errors that should not be caused by my change...
https://travis-ci.org/charlesdong1991/pandas/jobs/454724363, is it because the master branch has been updated? should i do git reset to catch the latest master before pushing?

charlesdong1991 · 2018-11-14T20:26:17Z

i removed the test part, and the error remains the same
(https://travis-ci.org/charlesdong1991/pandas/jobs/455118383), which looks from pyarrow import feather and others imported from pyarrow are wrong... do you have any thoughts how these errors appear? thanks so much, and sorry for the problems i bring... @datapythonista

datapythonista · 2018-11-14T23:21:46Z

Can't check the travis log from my phone, but it happens some times that things fail because of things unrelated to your PR.

Just leave your changes ready, and merge master in to your branch after some hours since the failure, and see if it's fixed.

Sometimes it can be that Anaconda is failing to download, a dependency released a new version that breaks something, problems in the Travis servers, something broken merged into master...

codecov · 2018-11-15T08:25:50Z

Codecov Report

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

@@            Coverage Diff             @@
##           master   #23650      +/-   ##
==========================================
+ Coverage   92.24%   92.25%   +<.01%     
==========================================
  Files         161      161              
  Lines       51336    51383      +47     
==========================================
+ Hits        47357    47404      +47     
  Misses       3979     3979

Flag	Coverage Δ
#multiple	`90.64% <ø> (ø)`	⬆️
#single	`42.31% <ø> (-0.03%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/formats/format.py	`97.87% <0%> (-0.01%)`	⬇️
pandas/core/strings.py	`98.58% <0%> (ø)`	⬆️
pandas/plotting/_core.py	`83.63% <0%> (ø)`	⬆️
pandas/core/indexing.py	`93.87% <0%> (ø)`	⬆️
pandas/core/generic.py	`96.82% <0%> (ø)`	⬆️
pandas/core/arrays/categorical.py	`95.34% <0%> (ø)`	⬆️
pandas/core/window.py	`96.4% <0%> (ø)`	⬆️
pandas/core/arrays/datetimes.py	`98.47% <0%> (ø)`	⬆️
pandas/core/frame.py	`97.02% <0%> (ø)`	⬆️
pandas/core/panel.py	`97.92% <0%> (+0.01%)`	⬆️
... and 3 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 c55057f...f20fee1. Read the comment docs.

charlesdong1991 · 2018-11-15T09:53:11Z

Hi, @datapythonista from my side, it looks like it passed all,
https://travis-ci.org/charlesdong1991/pandas, however, the check on master still fails
see https://travis-ci.org/pandas-dev/pandas/jobs/455383112, so can you take a look at what happens on master, and suggest a solution of how this can be fixed? thank you very much!

datapythonista

Looks good, couple of formatting things

datapythonista · 2018-11-15T20:07:35Z

scripts/validate_docstrings.py

@@ -733,6 +733,9 @@ def validate_all(prefix):
        If provided, only the docstrings that start with this pattern will be
        validated. If None, all docstrings will be validated.

+    ignore_deprecated: Boolean variable


Suggested change

ignore_deprecated: Boolean variable

ignore_deprecated : bool, default False

thanks for the correction!

datapythonista · 2018-11-15T20:07:44Z

scripts/validate_docstrings.py

@@ -733,6 +733,9 @@ def validate_all(prefix):
        If provided, only the docstrings that start with this pattern will be
        validated. If None, all docstrings will be validated.



Remove this blank line

datapythonista · 2018-11-15T20:08:48Z

scripts/tests/test_validate_docstrings.py

-            validate_docstrings, 'validate_all', lambda prefix: {
+            validate_docstrings, 'validate_all',
+            lambda prefix, ignore_deprecated=None:
+            {


Move the curly bracket to the previous line

datapythonista · 2018-11-15T20:51:17Z

scripts/tests/test_validate_docstrings.py

        assert exit_status == 0

    def test_exit_status_errors_for_validate_all(self, monkeypatch):
        monkeypatch.setattr(
-            validate_docstrings, 'validate_all', lambda prefix: {
+            validate_docstrings, 'validate_all',
+            lambda prefix, ignore_deprecated=None: {


Sorry I didn't see that before... ignore_deprecated expects a boolean value, I don't think we should pass None. And I see that it has a default value of False that is what we want. So, can you restore the original code? I think it should work, right?

ahh, i did this because i thought right now, validate_all takes in two arguments, and although ignore_deprecated has beed set to False by default, when we use lambda to create this mock data, we still need to assign a variable as a kind of placeholder, otherwise, there will be an error raised... can I still keep this, and remove None so that it looks like lambda: prefix, ignore_deprecated , please share your thoughts! And thanks for the quick response! @datapythonista

oh, that's right, I checked it fast and didn't see those are the arguments of the mock function, not the parameters being passed. In that case set ignore_deprecated=False, so we have the same signature in the mock as in the mocked function.

oh, yeah, changed to False! thx for the feedback!

datapythonista · 2018-11-15T20:51:37Z

scripts/tests/test_validate_docstrings.py

        assert exit_status == 5

    def test_no_exit_status_noerrors_for_validate_all(self, monkeypatch):
        monkeypatch.setattr(
-            validate_docstrings, 'validate_all', lambda prefix: {
+            validate_docstrings, 'validate_all',
+            lambda prefix, ignore_deprecated=None: {


same here, and in the rest of cases

datapythonista

lgtm, let's see if all tests pass now.

Thanks for the work on this @charlesdong1991

charlesdong1991 · 2018-11-16T07:39:06Z

@datapythonista yeah, it passed! thx very much for your help!!

datapythonista · 2018-11-16T11:42:23Z

Tanks for the contribution @charlesdong1991. I'll let another maintainer double check the changes and merge them, but all good from my side.

jreback · 2018-11-17T23:27:10Z

@jorisvandenbossche or @TomAugspurger

jorisvandenbossche

Looks good to me!

…fixed * upstream/master: (46 commits) DEPS: bump xlrd min version to 1.0.0 (pandas-dev#23774) BUG: Don't warn if default conflicts with dialect (pandas-dev#23775) BUG: Fixing memory leaks in read_csv (pandas-dev#23072) TST: Extend datetime64 arith tests to array classes, fix several broken cases (pandas-dev#23771) STYLE: Specify bare exceptions in pandas/tests (pandas-dev#23370) ENH: between_time, at_time accept axis parameter (pandas-dev#21799) PERF: Use is_utc check to improve performance of dateutil UTC in DatetimeIndex methods (pandas-dev#23772) CLN: io/formats/html.py: refactor (pandas-dev#22726) API: Make Categorical.searchsorted returns a scalar when supplied a scalar (pandas-dev#23466) TST: Add test case for GH14080 for overflow exception (pandas-dev#23762) BUG: Don't extract header names if none specified (pandas-dev#23703) BUG: Index.str.partition not nan-safe (pandas-dev#23558) (pandas-dev#23618) DEPR: tz_convert in the Timestamp constructor (pandas-dev#23621) PERF: Datetime/Timestamp.normalize for timezone naive datetimes (pandas-dev#23634) TST: Use new arithmetic fixtures, parametrize many more tests (pandas-dev#23757) REF/TST: Add more pytest idiom to parsers tests (pandas-dev#23761) DOC: Add ignore-deprecate argument to validate_docstrings.py (pandas-dev#23650) ENH: update pandas-gbq to 0.8.0, adds credentials arg (pandas-dev#23662) DOC: Improve error message to show correct order (pandas-dev#23652) ENH: Improve error message for empty object array (pandas-dev#23718) ...

…dev#23650)

charlesdong1991 mentioned this pull request Nov 12, 2018

CI: Add --ignore-deprecated to validate_docstrings.py #23631

Closed

datapythonista added Docs CI Continuous Integration labels Nov 12, 2018

datapythonista changed the title ~~add ignore-deprecate argument and add pytest~~ DOC: add ignore-deprecate argument and add pytest Nov 12, 2018

datapythonista changed the title ~~DOC: add ignore-deprecate argument and add pytest~~ DOC: Add ignore-deprecate argument to validate_docstrings.py Nov 12, 2018

datapythonista reviewed Nov 12, 2018

View reviewed changes

datapythonista approved these changes Nov 12, 2018

View reviewed changes

datapythonista mentioned this pull request Nov 14, 2018

DOC: Fix double blank lines in Series docstrings #23632

Closed

charlesdong1991 added 9 commits November 15, 2018 09:23

add ignore-deprecate argument and add pytest

e1fc4d5

fix flake8

a08cb6d

correct argument name

42e372f

code changed based on review

e569cf2

change name and help description

09be5aa

add ignore_deprecated description for functiondocstring

e7a723b

debug the error

70b0fcb

debug the error

66b9de3

set default value for ignore-deprecated

f5d28fe

charlesdong1991 added 9 commits November 15, 2018 09:23

add argument for setattr

1095a9a

correct argument name

26e1d04

assign true to ignore deprecated

b199099

debug the error

b384e79

correct the assert value

a0f9d21

modify pytest

af35c19

rewrite pytest

f1a66a6

remove added part to test if the failure caused by my code

79e5706

put tests back

83cc28c

charlesdong1991 force-pushed the add_ignore_deprecated branch from e6cb8d0 to 83cc28c Compare November 15, 2018 08:25

datapythonista requested changes Nov 15, 2018

View reviewed changes

charlesdong1991 added 2 commits November 15, 2018 21:16

remove curly bracket position

1ed082a

change formatting

14e522a

datapythonista reviewed Nov 15, 2018

View reviewed changes

charlesdong1991 added 2 commits November 15, 2018 21:59

remove redunctant default value

2418c0c

set default to False

f20fee1

datapythonista approved these changes Nov 15, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Nov 17, 2018

jorisvandenbossche approved these changes Nov 18, 2018

View reviewed changes

jorisvandenbossche merged commit 1250500 into pandas-dev:master Nov 18, 2018

tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018

DOC: Add ignore-deprecate argument to validate_docstrings.py (pandas-…

ca6cb49

…dev#23650)

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

DOC: Add ignore-deprecate argument to validate_docstrings.py (pandas-…

b2c4a66

…dev#23650)

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

DOC: Add ignore-deprecate argument to validate_docstrings.py (pandas-…

c70bbbc

…dev#23650)

	ignore_deprecated: Boolean variable
	ignore_deprecated : bool, default False

		@@ -733,6 +733,9 @@ def validate_all(prefix):
		If provided, only the docstrings that start with this pattern will be
		validated. If None, all docstrings will be validated.

DOC: Add ignore-deprecate argument to validate_docstrings.py #23650

DOC: Add ignore-deprecate argument to validate_docstrings.py #23650

Conversation

charlesdong1991 commented Nov 12, 2018 • edited by datapythonista Loading

pep8speaks commented Nov 12, 2018 • edited Loading

Comment last updated on November 13, 2018 at 12:55 Hours UTC

datapythonista commented Nov 12, 2018

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

datapythonista left a comment

Choose a reason for hiding this comment

charlesdong1991 commented Nov 12, 2018

charlesdong1991 commented Nov 13, 2018

datapythonista commented Nov 13, 2018

charlesdong1991 commented Nov 13, 2018

datapythonista commented Nov 13, 2018

charlesdong1991 commented Nov 13, 2018 • edited Loading

datapythonista commented Nov 13, 2018

charlesdong1991 commented Nov 14, 2018

charlesdong1991 commented Nov 14, 2018

datapythonista commented Nov 14, 2018

codecov bot commented Nov 15, 2018 • edited Loading

Codecov Report

charlesdong1991 commented Nov 15, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlesdong1991 Nov 15, 2018 • edited Loading

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

charlesdong1991 commented Nov 16, 2018

datapythonista commented Nov 16, 2018

jreback commented Nov 17, 2018

jorisvandenbossche left a comment

Choose a reason for hiding this comment

charlesdong1991 commented Nov 12, 2018 •

edited by datapythonista

Loading

pep8speaks commented Nov 12, 2018 •

edited

Loading

charlesdong1991 commented Nov 13, 2018 •

edited

Loading

codecov bot commented Nov 15, 2018 •

edited

Loading

charlesdong1991 Nov 15, 2018 •

edited

Loading