Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
datapythonista opened this issue Nov 27, 2018 · 12 comments · Fixed by #24086
Closed

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

datapythonista opened this issue Nov 27, 2018 · 12 comments · Fixed by #24086

Comments

@datapythonista
Copy link
Member

When we build the documentation from the .rst files in doc/source/, we use a variable common_imports that is added as context for sphinx, and that is used in the whatsnew files.

But more or less the same imports and options that are used in the whatsnew files, are used in most .rst files.

What would be useful is:

  • Rename common_imports to header, as this will be used for other things in the future (like defining tocdepth)
  • Add it at the beginning of all the .rst files in the same way as it's done for the whatsnew files
  • Remove the equivalent code already at the beginning of the files
  • If there is anything that would be useful to add to the content of common_imports (because it's used in many files), add it

This can have implications on the flake8-rst validation, as I assume the content is validated before rendering, and the imports will be missing during the validation.

CC: @TomAugspurger @FHaase

@FHaase
Copy link
Contributor

FHaase commented Nov 28, 2018

If the contents are the same throughout each file, they can be supplied via bootstrap option.

@saurav2608
Copy link

I am claiming this.

@saurav2608
Copy link

I understand what is required to complete this. But, how should I do the PR? This will require changing about 50 .rst files under 'whatsnew' and another 50 files in doc/source. Given that there are so many files involved, touching all of them would lead to conflicts. I am outlining proposed steps below -

  1. Change the variable common_imports to headers in conf.py and update all the whatsnew files to use this new variable instead of common_imports.
  2. Create a superset of all imports across all the rst files. (Note: this list should include the generic imports. Any special imports should remain in the respective .rst files closer to place they are referred.
  3. Move this superset to the headers variable in conf.py.
  4. Remove the top header from all rst files, retaining the special imports.

Let me know your thoughts. I think steps 1&2 can be clubbed into one PR.

@datapythonista
Copy link
Member Author

Feel free to do it in more than one PR, that's all right. But I wouldn't worry much about conflicts, they are not so likely to happen, as the changes to the files will be very small. And if in some case someone is cleaning that rst file and reorder the imports (that's the main thing that could generate a conflict), it should be quite easy to fix. Besides that, I'll try to merge it asap.

Just to be clear, if I'm not missing anything, the PR should mainly replace in all rst files:

.. currentmodule:: pandas
  
.. ipython:: python
    :suppress:
  
    import numpy as np
    import pandas as pd
    np.set_printoptions(precision=4, suppress=True)
    pd.options.display.max_rows = 15

by

{{ header }}

But as said, feel free to open more than one PR if that makes sense to you.

@datapythonista
Copy link
Member Author

@saurav2608 we just merged few changes in rst pages, if you can update the copy of pandas you're working with, I think that will prevent any possible conflict with the changes already done (and I don't think there will be more before merging your PR).

@saurav2608
Copy link

This change introduces additional bunch of erros on LINT=1 ./ci/code_checks.sh of the type F821 undefined name 'pd'

@datapythonista
Copy link
Member Author

I think you need to update the version of flake8-rst.

@FHaase
Copy link
Contributor

FHaase commented Dec 4, 2018

Add --bootstrap='import numpy as np; import pandas as pd' in ./ci/code-checks.sh

@datapythonista
Copy link
Member Author

We have it in the header, and the header is used in all the files, I don't think that should be necessary, right? Doesn't flake8-rst 0.7 merges all the blocks, including the first one with the header?

@datapythonista
Copy link
Member Author

Oh, sorry, just realized that flake8-rst is not rendering the header with its content, I forgot about it.

@FHaase can the bootstrap argument be added to setup.cfg, or should it be in the call?

@saurav2608 for the other imports failing, there are not many, and I think if you import StringIO... in the first block where there are used, just with few imports they should all be fixed. Do you mind doing that in the same PR?

@saurav2608
Copy link

saurav2608 commented Dec 4, 2018

I think the bootstrap argument is to be added to ./ci/code_checks.sh as
MSG='Linting code-blocks in .rst documentation' ; echo $MSG flake8-rst doc/source --filename=*.rst --format="$FLAKE8_FORMAT" --bootstrap 'import numpy as np; import pandas as pd'

If I add bootstrap to the file, then I am left with about 25 linting error most of them from cookbook.rst. And almost all of them can be taken care by importing the following packages - 'itertools', 'glob', 'os', and 'StringIO'.

We will also get a couple of errors from contributing.rst & comparison_with_sas.rst of the type 'F811 redefinition of unused 'np''

I can add a :flake8-add-ignore: F811 to the respective blocks.

@FHaase
Copy link
Contributor

FHaase commented Dec 4, 2018

flake8-rst works with the raw files. So no 'preprocessing' is done.

However I think I correct my comment from above. As we basically assume numpy and pandas are imported in both, the documentation and the docstrings i think it's better to add it in the setup.cfg.

bootstrap =
   import numpy as np
   import pandas as pd

can be taken care by importing the following packages - 'itertools', 'glob', 'os', and 'StringIO'.

Haven't they been added previously in the first .. ipython:: directive?
I'd leave a .. ipython:: directive with just those imports for now at that respective position and move those imports to the block where it's first needed later.

I can add a :flake8-add-ignore: F811 to the respective blocks.

Roles aren't supported currently, just use a # noqa: F811

@jreback jreback added this to the 0.24.0 milestone Dec 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants