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

Add version switcher to discretize docs #371

Merged
merged 4 commits into from
Sep 6, 2024
Merged

Add version switcher to discretize docs #371

merged 4 commits into from
Sep 6, 2024

Conversation

santisoler
Copy link
Member

Configure PyData Sphinx Theme's version switcher and add a new docs/_static/versions.json file where versions and their respective urls are listed.

Configure PyData Sphinx Theme's version switcher and add a new
`docs/_static/versions.json` file where versions and their respective
urls are listed.
Copy link

codecov bot commented Sep 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.25%. Comparing base (bd5bfa4) to head (b77a09c).
Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #371   +/-   ##
=======================================
  Coverage   86.25%   86.25%           
=======================================
  Files          89       89           
  Lines       18695    18695           
  Branches     2959     2959           
=======================================
  Hits        16125    16125           
  Misses       1884     1884           
  Partials      686      686           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@santisoler santisoler marked this pull request as ready for review September 3, 2024 23:19
@santisoler
Copy link
Member Author

@jcapriot let me know what do you think. Feel free to modify the versions.json directly according to the old versions we want to list in the switcher.

@jcapriot
Copy link
Member

jcapriot commented Sep 4, 2024

Let's keep it referenced by the actual full name of the tag, so including the "v".
I also adjusted the conf.py a bit so as to determine the version information once a bit further up

@jcapriot
Copy link
Member

jcapriot commented Sep 4, 2024

Leave the "v" in the "version" section of the switcher json and the "url", (you can remove it in the name).
It'll make it easier to point to the correct branch in the deploy step, as you can just access the Build.SourceBranchName in #372, instead of going about running a python script to get the version.

Copy link
Member Author

@santisoler santisoler left a comment

Choose a reason for hiding this comment

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

Sorry for stepping over your toes in that commit (I didn't see you change). I left two comments explaining why we actually need to remove the leading v.

},
{
"name": "0.10.0 (latest)",
"version": "v0.10.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, the version field should not have the leading v because these versions should be parsable by the compare-versions node module (https://pydata-sphinx-theme.readthedocs.io/en/stable/user_guide/announcements.html#version-warning-banners).

This way, the warning version banners can behave properly. If we have the v, any previous version is recognized as a development version, so the banner shows the wrong text.

We can still use the leading v in the url though.

Copy link
Member

Choose a reason for hiding this comment

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

compare_versions ignores any leading v
https://www.npmjs.com/package/compare-versions

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually experienced a misbehaviour in the version banner in simpeg before I changed the versions to not have a leading v. But yes, you're right that they seem to support it...

Ignore this then and we'll see if it works well.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like the pydata sphinx theme uses the leading v in their's for matching, https://github.com/pydata/pydata-sphinx-theme/blob/main/docs/_static/switcher.json

Copy link
Member Author

Choose a reason for hiding this comment

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

Then maybe there was something else that was failing back then... Well, I'm glad we don't have to fix it.

if discretize_version.is_devrelease:
branch = "main"
else:
branch = f"v{version}"
Copy link
Member Author

@santisoler santisoler Sep 5, 2024

Choose a reason for hiding this comment

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

We should also remove the leading v from here for the version to be parsable.

Suggested change
branch = f"v{version}"
branch = version

Copy link
Member

@jcapriot jcapriot left a comment

Choose a reason for hiding this comment

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

Let's do it!

@jcapriot jcapriot merged commit 707e31f into main Sep 6, 2024
25 checks passed
@jcapriot jcapriot deleted the version-switcher branch September 6, 2024 18:34
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.

2 participants