DOC: Use a standard header for all rst files #24086

saurav2608 · 2018-12-04T08:50:54Z

closes DOC: Use a standard header for all rst files #23952
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2018-12-04T08:50:57Z

Hello @saurav2608! Thanks for submitting the PR.

There are no PEP8 issues in the file doc/source/conf.py !

codecov · 2018-12-04T09:27:24Z

Codecov Report

Merging #24086 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #24086   +/-   ##
=======================================
  Coverage   42.51%   42.51%           
=======================================
  Files         161      161           
  Lines       51691    51691           
=======================================
  Hits        21975    21975           
  Misses      29716    29716

Flag	Coverage Δ
#single	`42.51% <ø> (ø)`	⬆️

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 940104e...ecba524. Read the comment docs.

codecov · 2018-12-04T09:27:24Z

Codecov Report

Merging #24086 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #24086   +/-   ##
=======================================
  Coverage   92.21%   92.21%           
=======================================
  Files         162      162           
  Lines       51723    51723           
=======================================
  Hits        47694    47694           
  Misses       4029     4029

Flag	Coverage Δ
#multiple	`90.61% <ø> (ø)`	⬆️
#single	`43.03% <ø> (ø)`	⬆️

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 949b148...1704878. Read the comment docs.

datapythonista

Looks quite good, if you can just revert the removal of datetime., and check the failures.

@FHaase do you want to take a look here?

datapythonista · 2018-12-04T12:54:14Z

doc/source/cookbook.rst

@@ -1333,20 +1314,20 @@ The :ref:`Timedeltas <timedeltas.timedeltas>` docs.

   s.max() - s

-   s - datetime.datetime(2011, 1, 1, 3, 5)


can you revert those, the import should be import datetime

datapythonista · 2018-12-04T12:58:06Z

doc/source/conf.py

+   import functools
+   import glob
+   import itertools
+   import os


All those shouldn't be here, should be visibly imported in the places where they are used.

Can you check if we the currently validated flake8-rst files (the ones not ignored in setup.cfg), removing them raises an error?

That would apply to all the imports except import pandas as pd and import numpy as np.

Don't fix the errors in this PR if there are more than couple, but if you can show how many failures are without the imports, that would be great.

these are also done.

datapythonista · 2018-12-05T17:27:42Z

@saurav2608 will you have time to add the numpy and pandas imports to setup.cfg, remove the others from the header, and add the needed ones to where they are used?

FHaase · 2018-12-05T18:04:02Z

@FHaase do you want to take a look here?

Once your suggestions are implemented I'm fine with that.

@saurav2608 adding this to setup.cfg in the [flake8-rst] section should fix most of the F821 issues.

[flake8-rst]
bootstrap = 
    import numpy as np
    import pandas as pd
...

saurav2608 · 2018-12-05T20:33:26Z

@saurav2608 will you have time to add the numpy and pandas imports to setup.cfg, remove the others from the header, and add the needed ones to where they are used?

I will definitely complete this. I am traveling for work, but will get this done by tomorrow.

jreback · 2018-12-05T20:51:49Z

@datapythonista I assume you want to merge this before anything else touches the .rst's themselves?

datapythonista · 2018-12-05T21:22:54Z

I don't think we have anything else modifying the .rst headers, but if we do would be nice to wait a bit and avoid conflicts in this PR, yes.

jorisvandenbossche

@saurav2608 Thanks for working on this!

jorisvandenbossche · 2018-12-06T21:46:17Z

doc/source/conf.py

+   import glob
+   import itertools
+   import os
+
   import numpy as np
   from pandas import *


This from pandas import * should also not be here. If it was needed for the whatsnew files (where the common imports were used now?), I would still keep that separate until we fix the examples in the whatsnew files to not need this.

This from pandas import * should also not be here. If it was needed for the whatsnew files (where the common imports were used now?), I would still keep that separate until we fix the examples in the whatsnew files to not need this.

Okay - I have added from pandas import * to all whatsnew files. We have to revisit all the whatnew files.

I have added from pandas import * to all the whatsnew file. We have to review all of them later to see where files actually need the import (my guess is all of them).

datapythonista · 2018-12-07T11:57:41Z

@saurav2608 did you push your changes? I still can see all the imports in header. The only imports in header should be import numpy as np and import pandas as pd. Thanks!

saurav2608 · 2018-12-07T11:59:01Z

@saurav2608 did you push your changes? I still can see all the imports in header. The only imports in header should be import numpy as np and import pandas as pd. Thanks!

No I have not pushed it yes. I am making the changes now.

datapythonista

looks great, just couple of questions, not sure if I'm missing something

datapythonista · 2018-12-07T15:07:38Z

doc/source/comparison_with_sas.rst

-    import pandas as pd
-    import numpy as np
+    import pandas as pd    # noqa: F811
+    import numpy as np     # noqa: F811


why don't we use the header here? you can add the F811 to the --bootstrap parameter value if needed

This is a tutorial of sorts and the tutorial walks thru the process of importing pandas and numpy visibly (not with a :supress: ). I think without the visible import of pandas the tutorial will not make sense.

Makes sense. But can you still include the header in those files? Besides the imports, there is other stuff. And we will potentially add more, and expect to be applied to 100% of rst files. Thanks!

datapythonista · 2018-12-07T15:08:04Z

doc/source/contributing.rst

-   import pandas as pd
-   from pandas.util import testing as tm
+   import numpy as np        # noqa: F811
+   import pandas as pd       # noqa: F811


This similar to the issue above. The tutorial presents a complete block of code to be copied and saved as a .py file. Without the explicit import, anyone following the tutorial will not be able to replicate the results. It might make sense to include this block as a non executable block.

datapythonista · 2018-12-07T15:08:26Z

doc/source/install.rst

@@ -207,7 +207,7 @@ installed), make sure you have `pytest

 ::

-    >>> import pandas as pd
+    >>> import pandas as pd  # noqa: F811


On this I agree with you. I can remove the explicit import.

datapythonista · 2018-12-07T16:11:59Z

what I don't understand is why the noqa F811 is needed. In header, after the import pandas as pd, we use pd to set options, don't we? And I think for numpy we set the seed. So, F811 shouldn't be raised if the module is being used after the import and before the repeated one, or should it?

CC: @FHaase

saurav2608 · 2018-12-07T16:32:41Z

There is another behavior I am unable to understand. This build fails on linting error on the file comparison_with_sql.rst. comparison_with_sql.rst:17:5: F811 redefinition of unused 'pd' from line 2. This file does not have the header block (I have not touched this file in this commit). Thus pd is defined for the first time at line 17. Is the bootstrap parameter causing F811?

jorisvandenbossche · 2018-12-07T16:41:04Z

@saurav2608 general note: when updating the PR, can you add new commits instead of amending or squashing with the previous one? That makes it easier to review only the new changes.
(When the PR is merged, it will be squashed anyway, so it is no problem to have "fixup" commits).

datapythonista · 2018-12-07T16:42:48Z

@saurav2608, if you want you can remove the noqa, and if the files are being checked for flake8-rst and ci/code_checks.py reports errors, you can add the files to the ignore list for flake8-rst in setup.cfg.

This way we can merge this, avoid conflicts, and we can fix the flake8-rst errors in a new PR

saurav2608 · 2018-12-07T18:04:18Z

@datapythonista, I have removed the #noqa and added the files to the ignore list. I have also added header to all the rst file.

datapythonista

looks great, thanks a lot for working on this

do you mind creating yourself the follow up issues for:

Removing the from pandas import * in the whatsnew files
Fix the errors with flake8-rst that report errors in the import pandas as pd...
Remove the randn = numpy.random.randn and replace the randn by np.random.randn

Thanks!

saurav2608 · 2018-12-07T19:02:22Z

Sure, will open the issues.

jreback · 2018-12-07T21:18:07Z

@datapythonista looks ok. are you sure you wan tthe from pandas import * in the older release notes? or just not worth it to fix

datapythonista · 2018-12-07T21:54:22Z

I think for now makes sense to have the from pandas import *, so we can validate the rest of the files for errors (and avoid ignoring them and not validate any error). I guess there won't be many errors, and if that's the case it's probably worth to leave them clean at some point (surely not a priority).

saurav2608 · 2018-12-09T15:39:33Z

@datapythonista , @jreback : Can you please merge this? I will create follow-up issues for the revised code.

jreback · 2018-12-09T15:48:54Z

@saurav2608 pls merge master. @datapythonista ok by me.

datapythonista · 2018-12-09T16:16:47Z

@saurav2608 merged the conflict myself, merging on green

datapythonista · 2018-12-09T17:53:26Z

Thanks @saurav2608 for taking care of this, good improvement

datapythonista · 2018-12-09T19:13:02Z

@saurav2608 I already fixed the errors when importing import pandas as pd (and numpy) in the files in #24182, no need to open an issue for those

jorisvandenbossche · 2018-12-10T12:28:42Z

doc/source/cookbook.rst

@@ -1081,6 +1069,8 @@ Option 1: pass rows explicitly to skip rows

 .. ipython:: python

+    from pandas.compat import StringIO


Side note: in the public docs I think we should use the standard python from io import StringIO (as I don't think we should recommend people normal users to use pandas.compat)

datapythonista reviewed Dec 4, 2018

View reviewed changes

jreback added the Docs label Dec 4, 2018

jorisvandenbossche reviewed Dec 6, 2018

View reviewed changes

datapythonista self-assigned this Dec 7, 2018

DOC: Use standard header for .rst files

3cc9f67

saurav2608 force-pushed the rst-header branch from ecba524 to 3cc9f67 Compare December 7, 2018 14:33

datapythonista reviewed Dec 7, 2018

View reviewed changes

DOC: Std header for rst. Remove #noqa

27eab4a

datapythonista approved these changes Dec 7, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Dec 7, 2018

Merge branch 'master' into rst-header

1704878

datapythonista merged commit 3bc2831 into pandas-dev:master Dec 9, 2018

saurav2608 deleted the rst-header branch December 9, 2018 18:00

datapythonista mentioned this pull request Dec 10, 2018

DOC: Adding header to the rst files that don't have it #24191

Merged

jorisvandenbossche reviewed Dec 10, 2018

View reviewed changes

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

DOC: Use a standard header for all rst files (pandas-dev#24086)

257a7e1

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

DOC: Use a standard header for all rst files (pandas-dev#24086)

a1a644c

		@@ -1333,20 +1314,20 @@ The :ref:`Timedeltas <timedeltas.timedeltas>` docs.

		s.max() - s

		s - datetime.datetime(2011, 1, 1, 3, 5)

		@@ -1081,6 +1069,8 @@ Option 1: pass rows explicitly to skip rows

		.. ipython:: python

		from pandas.compat import StringIO

DOC: Use a standard header for all rst files #24086

DOC: Use a standard header for all rst files #24086

Conversation

saurav2608 commented Dec 4, 2018

pep8speaks commented Dec 4, 2018

codecov bot commented Dec 4, 2018

Codecov Report

codecov bot commented Dec 4, 2018 • edited Loading

Codecov Report

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 commented Dec 5, 2018

FHaase commented Dec 5, 2018

saurav2608 commented Dec 5, 2018

jreback commented Dec 5, 2018

datapythonista commented Dec 5, 2018

jorisvandenbossche 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

datapythonista commented Dec 7, 2018

saurav2608 commented Dec 7, 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

datapythonista commented Dec 7, 2018

saurav2608 commented Dec 7, 2018

jorisvandenbossche commented Dec 7, 2018

datapythonista commented Dec 7, 2018 • edited Loading

saurav2608 commented Dec 7, 2018

datapythonista left a comment

Choose a reason for hiding this comment

saurav2608 commented Dec 7, 2018

jreback commented Dec 7, 2018

datapythonista commented Dec 7, 2018

saurav2608 commented Dec 9, 2018

jreback commented Dec 9, 2018

datapythonista commented Dec 9, 2018

datapythonista commented Dec 9, 2018

datapythonista commented Dec 9, 2018

Choose a reason for hiding this comment

codecov bot commented Dec 4, 2018 •

edited

Loading

datapythonista commented Dec 7, 2018 •

edited

Loading