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

adding "sphinx_rtd_theme" to conf.py #531

Merged
merged 5 commits into from
Oct 6, 2023
Merged

adding "sphinx_rtd_theme" to conf.py #531

merged 5 commits into from
Oct 6, 2023

Conversation

alex-rakowski
Copy link
Collaborator

looks like this needed to be added to the extensions in conf.py

https://sphinx-rtd-theme.readthedocs.io/en/stable/installing.html

@alex-rakowski alex-rakowski requested a review from sezelt October 6, 2023 10:30
@sezelt
Copy link
Member

sezelt commented Oct 6, 2023

Since this adds a dependency I think we should get a vibecheck from @bsavitzky.

Alternatively, do you think it's possible to make this an optional dependency, like py4dstem[docs] and have the readthedocs builder install that option? That way ordinary installs don't require sphinx_rtd_theme.

@alex-rakowski
Copy link
Collaborator Author

I moved the dependency from the setup.py and put it in the docs requirement.txt instead. If we wanted a pip install py4DSTEM[docs] method instead we'd have to alter setup.py and .readthedocs.yml with

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

Copy link
Member

@sezelt sezelt left a comment

Choose a reason for hiding this comment

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

Having the requirement within the docs setup file seems like the best solution, since it doesn't interfere with the requirements for installing and running.
Thanks

@sezelt sezelt merged commit 69df1a4 into dev Oct 6, 2023
7 checks passed
@sezelt sezelt deleted the docs branch October 6, 2023 19:35
@bsavitzky
Copy link
Member

bsavitzky commented Oct 9, 2023 via email

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