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

Update docs to use custom theme #29

Merged
merged 7 commits into from
Oct 15, 2020
Merged

Update docs to use custom theme #29

merged 7 commits into from
Oct 15, 2020

Conversation

ewu63
Copy link
Collaborator

@ewu63 ewu63 commented Oct 12, 2020

Purpose

We created a custom theme which is published on pyPI, to help make all the documentation sites consistent. This accomplishes several things

  • Shared theme styles without modifying each repo
  • The CSS table fix can be placed in a single place
  • Default options for conf.py can be set globally, and imported in each repo without having to change the setting for each repo, e.g. the copyright year

Type of change

What types of change is it?
Select the appropriate type(s) that describe this PR

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

No need

Checklist

Put an x in the boxes that apply.

  • I have run flake8 and black to make sure the code adheres to PEP-8 and is consistently formatted
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@ewu63 ewu63 requested a review from a team as a code owner October 12, 2020 20:41
@ewu63 ewu63 marked this pull request as draft October 12, 2020 20:41
@ewu63 ewu63 marked this pull request as ready for review October 14, 2020 17:18
marcomangano
marcomangano previously approved these changes Oct 14, 2020
Copy link
Contributor

@marcomangano marcomangano left a comment

Choose a reason for hiding this comment

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

I double checked this with the MDO Lab theme code to understand the changes. It makes a lot of sense to me, and I am fine with using this PR as a blueprint to update the other packages.

@ewu63
Copy link
Collaborator Author

ewu63 commented Oct 14, 2020

@marcomangano @sseraj, what I mean by I'm not sure about the imports is that, even more stuff can be imported:

  • autodoc_mock_imports could by default contain things like numpy and scipy
  • html_theme could be imported too, which is a bit funny but will work.

Maybe it doesn't matter, and this PR as-is is more explicit.

@marcomangano
Copy link
Contributor

marcomangano commented Oct 14, 2020

Oh yeah @nwu63 , importing the html_theme is meta but it makes sense. About the autodoc_mock_imports, unless you want to import all the modules directly in the theme, I am fine with leaving them in the repo conf.py, together with the extensions. It makes sense for me as they are repo dependent and, even if some modules like numpy are probably coming up almost everywhere, it might be overly convoluted to have these imports in two separate arrays.
Anyway, we can go over a first refactoring of all the other repos, and move some autodoc_mock_imports into the theme at a later stage.

@ewu63
Copy link
Collaborator Author

ewu63 commented Oct 14, 2020

Yes agreed, and leaving html_theme in there is more explicit so I am fine with that. In that case, let's go ahead with this approach and we can iterate in the future as needed.

Copy link
Collaborator

@sseraj sseraj left a comment

Choose a reason for hiding this comment

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

Would adding html_theme to the global config.py cause problems in requirements.txt? If not, I am inclined to adding it there and importing it.

@ewu63
Copy link
Collaborator Author

ewu63 commented Oct 14, 2020

Nope, it should be fine. Alright, I'll update the package to make that available for import here.

@ewu63
Copy link
Collaborator Author

ewu63 commented Oct 14, 2020

So I ended up adding all the built-in sphinx extensions to config.py, I think that's relatively clean. The extra ones like numpydoc should be specified per repo, and of course you need to make sure it's in the requirements.txt file for RTD to install. This build should pass

Copy link
Collaborator

@sseraj sseraj left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good.

Copy link
Contributor

@marcomangano marcomangano left a comment

Choose a reason for hiding this comment

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

Looks very clean, let's extend this to the other repos!

@ewu63 ewu63 merged commit 4ce9140 into master Oct 15, 2020
@ewu63 ewu63 deleted the update-theme branch October 15, 2020 12:51
@marcomangano marcomangano mentioned this pull request Oct 16, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants