Deprecate get_html_theme_path()
#211
Conversation
Eric-Arellano
requested review from
1ucian0,
nonhermitian,
mtreinish and
HuangJunye
There was a problem hiding this comment.
I did a bit of rabbit hole digging. This method was copied from sphinx_rtd_theme and it seems that it was necessary for getting the path for deploying to readthedocs.org, otherwise readthedocs.org will use it's own theme (which I thought it would be sphinx_rtd_theme, wouldn't it?!). Anyway, this is not necessary for us, and in fact it's also not necessary for deploying to readthedocs.org (which is one of the deployment options for ecosystem projects) so we can safely remove this function.
|
I am not merging yet in case this blocked by the PRs that remove its usage in qiskit projects (I don't think so since this PR only adds deprecation, not removal). |
Exactly, and we can always push the deprecation to a later release if those deprecation PRs have not landed yet. Two has landed so far out of 5. Also, this won't impact anyone until we do a release of the theme So it should be okay to merge now |
<!--⚠️ If you do not respect this template, your pull request will be closed.⚠️ Your pull request title should be short detailed and understandable for all.⚠️ Also, please add it in the CHANGELOG file under Unreleased section.⚠️ If your pull request fixes an open issue, please link to the issue. ✅ I have added the tests to cover my changes. ✅ I have updated the documentation accordingly. ✅ I have read the CONTRIBUTING document. --> ### Summary The `qiskit_sphinx_theme` package will soon have "variants" that allow setting the theme to either Core (Terra) or Ecosystem. When that happens, `get_html_theme_path()` will no longer make semantic sense because we will have multiple HTML themes in the package, and the user needs to choose which they want. Turns out, setting `html_theme_path` isn't necessary in the first place. It looks like bad copy-and-paste from the Sphinx RTD theme. It has no impact on our 4 Applications consumers. See qiskit-community/qiskit-finance#244 for an example; the docs render the same before and after. ### Details and comments I am updating all known users of `qiskit_sphinx_theme` to stop using this function. (Most don't use it already, such as Terra)
Summary
The
qiskit_sphinx_themepackage will soon have "variants" that allow setting the theme to either Core (Terra) or Ecosystem.When that happens,
get_html_theme_path()will no longer make semantic sense because we will have multiple HTML themes in the package, and the user needs to choose which they want.Turns out, setting
html_theme_pathisn't necessary in the first place. It looks like bad copy-and-paste from the Sphinx RTD theme. It has no impact on our 4 Applications consumers. See qiskit-community/qiskit-finance#244 for an example; the docs render the same before and after.Details and comments
I am updating all known users of
qiskit_sphinx_themeto stop using this function. (Most don't use it already, such as Terra)