Skip to content
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

DOC: Improving code quality of doc/make.py, PEP-8 and refactoring (#19631) #19634

Merged
merged 1 commit into from
Feb 22, 2018
Merged

DOC: Improving code quality of doc/make.py, PEP-8 and refactoring (#19631) #19634

merged 1 commit into from
Feb 22, 2018

Conversation

datapythonista
Copy link
Member

@datapythonista datapythonista commented Feb 10, 2018

Summary of the PR:

@jreback jreback added the Docs label Feb 10, 2018
doc/make.py Outdated
cls._sphinx_build('html')

@classmethod
def zip_html(cls):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a lot of these are not used so can be removed

@TomAugspurger @jorisvandenbossche

IOW we only need html, latex, clean, can remove all of the upload* ones

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zip_html is also used.
I did use the upload commands when I uploaded the docs. Not sure how Tom currently does it (I know we changed some things in where we upload the versioned docs and how they are symlinked)

doc/make.py Outdated
"""Alias to build HTML by default."""
cls.html()

@classmethod
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we actually support this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the current script does, both python make.py and python make.py all are equivalent to python make.py html. Would rather get rid of all and would show the usage if no command is provided, but I tried to keep compatibility with the old script.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i understand but let's remove what we dont need instead

doc/make.py Outdated
cls.auto_dev(debug=True)

@classmethod
def build_pandas(cls):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when is this actually called?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current script supports python make.py build_pandas, not sure if anyone is using it, happy to get rid of it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never used it myself

@datapythonista
Copy link
Member Author

Updated the PR, I left html, latex, clean and zip_html.

Just let me know if something else should be kept.

@jorisvandenbossche
Copy link
Member

latex_forced is used as well I think

@TomAugspurger
Copy link
Contributor

TomAugspurger commented Feb 11, 2018 via email

@datapythonista
Copy link
Member Author

Updated the PR with latex_forced.

@TomAugspurger, we've got rid of make.py all, which was implemented as an alias of make.py html. But may be we can get it back to execute all the steps for the release: ./make.py clean; ./make.py; ./make.py zip_html; ./make.py latex_forced.

Just let me know if you think that's a good idea.

@jreback jreback added this to the 0.23.0 milestone Feb 22, 2018
@jreback jreback merged commit b27c541 into pandas-dev:master Feb 22, 2018
@jreback
Copy link
Contributor

jreback commented Feb 22, 2018

thanks @datapythonista

so this will prob need to have a followup with some instructions on how to use it? or is completely obvious from the --help? maybe a README.md?

@datapythonista
Copy link
Member Author

@jreback a README never hurts, and we could surely have some more info, but I think the usage is quite covered in:

I'm working in implementing a way to build a single document from the api (and pep8fying doc/source/conf.py which I forgot in this PR). I'll review the documentation about doc/make.py once I finish that, and add documentation for anything not explained.

@jorisvandenbossche
Copy link
Member

I'm working in implementing a way to build a single document from the api (

What do you already have? Because I updated yesterday my script I shared, and it is basically working (was planning to do a PR to add it this morning, but did not yet come to it)

@datapythonista
Copy link
Member Author

@jorisvandenbossche I'll share what I've got in a branch later today. But my approach was to create a temporary directory, copy the files to have a minimal sphinx project there, run sphinx on it, and then copy the single api page it generates to the real project.

I see 2 advantages in this approach, first I found it tricky to build a single page from the api from sphinx, and second I prefer to avoid deleting all the .rst files to build the project. Doing a git checkout on them later is not a big deal if you know about it, but newcomers may find it confusing.

@jreback
Copy link
Contributor

jreback commented Feb 22, 2018

you know —single already exists yes?

@datapythonista
Copy link
Member Author

@jreback thanks for double checking, but yes, I do. Actually I want to reuse it for simplicity of the interface. But now you can do something like ./make.py html --single contributing.rst but not something like ./make.py hml --single pandas.DataFrame.head which is what @jorisvandenbossche and myself were discussing, and which will be very convenient for the sprint.

@jorisvandenbossche
Copy link
Member

I was planning to remove this "OK to delete those files" as well, as it is indeed very annoying (and I think not needed, if you specify to sphinx that those files can be ignored, that's the approach I was taking)

I will put up my approach as well as a PR, and then you can test it and we can see which approach is the easiest for the end user.

@datapythonista
Copy link
Member Author

@jorisvandenbossche if you plan to remove the file deletion, which I think it's implemented in sphinx conf.py, can you please first review and merge #19839, so there are no conflicts with it later? It's a simple PR to make the file PEP-8 compatible.

If you can build a single api doc without deleting the rst, that's a surely much better approach than what I did. I didn't see how to do that. There are still couple of things missing, but this is my approach: datapythonista@5b4cc3b

@jorisvandenbossche
Copy link
Member

@datapythonista my attempt is here: #19840. I used exclude_patterns to not have to delete the files. This now seems to work smoothly (try eg python make.py --docstring DataFrame.join), apart from a problem I have with subprocess.check_call (will comment on the PR).
I also still can be smarter (now hardcoded 'method', but could in principle detect this from the passed method/function name, instead of requiring the user to specify this).

#19839 will not give too much conflict, I only deleted some lines from conf.py

harisbal pushed a commit to harisbal/pandas that referenced this pull request Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants