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

Users should set docs_url_prefix for Translations and Previous Releases #321

Merged
merged 8 commits into from
May 16, 2023

Conversation

Eric-Arellano
Copy link
Collaborator

Closes #303.

Everyone's local copy of versionutils.py already had the option content_prefix, which had to be set to e.g. ecosystem/finance or documentation. That was already being used by the Translations feature. (But it was only for their local projects; qiskit_sphinx_theme never had this option.)

This PR makes some improvements to content_prefix:

  • Renames to the more clear docs_url_prefix
  • Eagerly errors if it is not set and either Translations or Previous Releases are used
  • Refactors Previous Releases logic to use this option.
    • This results in simpler logic, and the URL will now be determined by Sphinx at build time, rather than by dynamic JavaScript code at runtime. That is valuable because it makes our <a href> an actual link, which better complies with the Semantic Web.

Copy link
Collaborator

@javabster javabster left a comment

Choose a reason for hiding this comment

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

Nice work so far!

from qiskit_sphinx_theme.previous_releases import get_previous_versions_url


def test_get_previous_versions_url() -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you add a docstring please :)

Also might be nice to parameterise this test? Its a small test so not a big deal if not, just a nice to have

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also also, there seem to be much fewer test cases covered here compared to the whole test file above that got deleted. Is there a reason why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also might be nice to parameterise this test? Its a small test so not a big deal if not, just a nice to have

Sure, done.

there seem to be much fewer test cases covered here compared to the whole test file above that got deleted. Is there a reason why?

Yeah, it's because this implementation is substantially simpler. We don't need to extract out what the docs_url_prefix is anymore using fancy regex on the current URL. The user already tells us now via conf.py. And we know the version from the Jinja template. So, all we're doing is combining those two values with an f-string.

tests/py/translations_test.py Show resolved Hide resolved
docs_url_prefix = "ecosystem/finance"
```

## Enable Previous Releases
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @coruscating - could you please read through these instructions?

Comment on lines -9 to -10
("", "index", "/index.html"),
("", "subdir/my_page", "/subdir/my_page.html"),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We now error if you set "" because it won't work for any project we have in the wild: they all have a prefix.

The only one I was confused by is Terra. They set this value to documentation, but in a weird way in some scripts that took me a while to find. So, at first, I incorrectly thought Terra was setting the value to "".

README.md Outdated
Then, update your `conf.py`:

* Ensure that `qiskit_sphinx_theme` is in the `extensions` setting.
* Add to the option `html_context` an entry for `version_list` with a list of the prior versions, e.g. `["0.4", "0.5"]`. Each of these versions must be deployed with the above `stable/<version>` URL scheme.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some projects use an auto-generated list: https://github.com/Qiskit/qiskit-experiments/blob/a8f9de8b39774ce2c22c0ec5ae19bd7f023bfeca/docs/conf.py#L185

Which method should be listed here as the standard?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I want to investigate in #307 if we can get more standardization for Previous Releases. In the meantime, it's up to each project to do this how they want.

I rewrote this section to give more context. Thanks for the feedback!


This feature allows you to link to previous versions of the docs in the left sidebar.

First, start additionally deploying your docs to `<project-prefix>/stable/<version>/`, e.g. `/ecosystem/finance/stable/0.5/index.html`. See https://github.com/Qiskit/qiskit-experiments/blob/227867937a08075092cd11756214bee3fb1d4d3d/tools/deploy_documentation.sh#L38-L39 for an example.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I'd prefer an inline rclone example here that works with your ecosystem/finance example rather than a link to a different repo's code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking it's better to link to a repo because it's a more complete example, such as showing how to determine what the version is. It seems like every project is using the same script to deploy docs, so I think this should be fine?

Copy link
Collaborator

@javabster javabster left a comment

Choose a reason for hiding this comment

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

LGTM, new tests look great! 🚀

just left one tiny optional suggestion

qiskit_sphinx_theme/__init__.py Outdated Show resolved Hide resolved
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.

Determine what we want to do with content_prefix option
3 participants