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

RTD sphinx format updates #141

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

RTD sphinx format updates #141

wants to merge 14 commits into from

Conversation

sean-lockwood
Copy link
Member

@sean-lockwood sean-lockwood commented Apr 8, 2022

This PR allows the RTD environment to build.
PRs #145 + #151 fixed the RTD build issue, but this PR resolves formatting issues in the docs.

Build was failing due to time-out or memory exhaustion on the RTD server. Now mocking many packages and selectively installing them based on the READTHEDOCS environment variable.

Sphinx issues rectified by modifying docstrings.

Also updated RTD environment to Python 3.7.

New build of docs available for review here:
https://stistools.readthedocs.io/en/sl_rtd-env/

vs current build:
https://stistools.readthedocs.io/en/latest/

Note that the TOC now displays various helper functions. We need to decide if this should be suppressed.

This should hopefully allow the RTD environment to build.
@sean-lockwood sean-lockwood self-assigned this Apr 8, 2022
Copy link
Contributor

@dougbrn dougbrn left a comment

Choose a reason for hiding this comment

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

Looks good. Feel free to merge

@sean-lockwood
Copy link
Member Author

I tried building the docs in a separate RTD version.
https://stistools.readthedocs.io/en/sl_rtd-env/index.html

Something happened to the formatting we should work out before merging. This might be caused by a change to the Sphinx version?

Before:
image

After:
image

@sean-lockwood sean-lockwood marked this pull request as draft April 8, 2022 20:32
…nal "string" to "str" in datatype descriptions.
@sean-lockwood
Copy link
Member Author

sphinx on RTD (4.5.0) is not generating bulleted lists properly. This may be related to the docutils version (0.17.1) needing to be pinned to 0.16 (or possibly a version >= 0.18).

@sean-lockwood
Copy link
Member Author

Still having problems with bulleted lists, e.g.:

image

@sean-lockwood sean-lockwood changed the title Changed RTD environment to build in python 3.7 Fixed RTD build Jan 12, 2023
@sean-lockwood sean-lockwood marked this pull request as ready for review January 12, 2023 20:29
@sean-lockwood
Copy link
Member Author

@starphire -
Looking for feedback on the formatting issues vs the current version. E.g., does the lack of bulleted lists change the meaning in any places?

@sean-lockwood
Copy link
Member Author

@stscirij -
The change to the setup.py install_requires seems a little hacky. Feedback would be appreciated.

@sean-lockwood
Copy link
Member Author

Note that PR #145 also changes the RTD configuration. Haven't resolved the differences yet.

@zacharyburnett
Copy link
Collaborator

zacharyburnett commented Jan 20, 2023

@stscirij -
The change to the setup.py install_requires seems a little hacky. Feedback would be appreciated.

Would adding the docs extra to readthedocs.yaml work?
https://github.com/spacetelescope/jwst/blob/f10a9bba8ca54a460f649e1de0af1c4efb1d5f87/.readthedocs.yaml#L20-L26

python:
  version: 3.8
  install:
    - method: pip
      path: .
      extra_requirements:
        - docs

EDIT: I apologize, I misread the PR; I thought the issue was selectively adding docs requirements, instead of removing them.

@sean-lockwood sean-lockwood changed the title Fixed RTD build RTD sphinx format updates Mar 7, 2023
@sean-lockwood
Copy link
Member Author

@stscirij @starphire -
I believe this branch is ready for review.

Copy link
Collaborator

@starphire starphire left a comment

Choose a reason for hiding this comment

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

Formatting looks good, and search bar removal probably is probably better off. Feel free to merge!

Copy link
Contributor

@stscirij stscirij left a comment

Choose a reason for hiding this comment

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

The numpy/scipy docstring format calls for a space between a parameter and the colon (and most of the changes in this PR involve removing that space). Not exactly sure why, but jwst follows this practice.

@sean-lockwood
Copy link
Member Author

@stscirij -

Ideally we shouldn't have to change the docstrings out of compliance. Just found something about the theme possibly causing problems with the colons in the parameter lists (stsci_rtd_theme in our case):

fsspec/filesystem_spec#1150

I'll look into this a bit more first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants