-
Notifications
You must be signed in to change notification settings - Fork 213
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
Updates documentation versioning #4613
Conversation
You can preview documentation at https://esmci.github.io/cime/branch/fix_docs_version/html/index.html |
TODO:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on the documentation @jasonb5 . I appreciate the goal of removing the dependence on the sphinx_rtd_theme fork, and at first I liked the simplicity of this new approach. But as I note below, I wonder if this means dropping one of the major goals of the original versioned documentation....
doc/source/_templates/versions.html
Outdated
</dl> | ||
{% endif %} | ||
<hr/> | ||
{% trans %}Free document hosting provided by <a href="http://www.readthedocs.org">Read the Docs</a>.{% endtrans %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing this ReadTheDocs advertisement appear at the bottom. Is that intentional? It's not a big deal, but if it isn't intentional I'm wondering if we should remove that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I'll remove this, came with the boilerplate code and we're not even hosting them on readthedocs.
doc/source/conf.py
Outdated
versions = ( | ||
("master", "master"), | ||
("CESM2.2", "cesm2.2"), | ||
("CESM2.1 (cime5.6)", "maint-5.6"), | ||
("ufs_release_v1.1", "ufs_release_v1.1"), | ||
("ufs_release_v1.0", "ufs_release_v1.0"), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand this right, I am concerned about this piece. A major goal of the original versioned documentation was to avoid needing to update every branch and rebuild all versions of the documentation whenever we add a new version. The current approach does that by building the version list dynamically with a bit of JavaScript... if I remember correctly that's done with https://github.com/ESMCI/cime/blob/master/doc/source/_static/pop_ver.js, using the versions listed in https://github.com/ESMCI/cime/blob/gh-pages/versions/versions.json. Note that the versions.json just exists in a single place, on the gh-pages branch, so adding a new version just requires updating that file on the gh-pages branch: no changes are needed to the doc source, and no rebuild is needed.
Am I understanding right that this new approach will require keeping this versions list up to date on all branches from which documentation is built, and then rebuilding the documentation from all branches whenever a new version is added? If so, I want to make sure that this is something that people are comfortable with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@billsacks Yes all versions built after this change won't have new versions listed but will have all previous versions. The master
documentation will have links to all versions and all versions will have a link to master
. This would break consistency if a user were to go master
-> cesm2.2
-> cesm2.3
where cesm2.3
was built after cesm2.2
.
If the above use case is a hard requirement I can look into supporting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know about a hard requirement, but I think this is worth at least having a discussion about (unless there already has been some discussion that I missed). If this can't be supported, I guess I'd like to hear a little more about what's motivating this change: were there problems supporting the sphinx_rtd_theme? I'm partly interested because we use this same approach throughout CESM, so I'm curious what problems might apply elsewhere as well and if we can come up with a consistent solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can look into supporting this being dynamic as before. As for the motivation, using the newest sphinx conflicted with the fork of sphinx_rtd_theme when building the docs, switching to the latest resolved the issue. Quick search showed a method to add a version selector using the latest version of the theme. Rather than continuing to maintain a fork of the theme, I thought it best to utilize the boiler code and remove the dependency on the fork.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, I was afraid that might happen. A bit of background, if I remember correctly: the fork I set up was based on a branch that someone created that (I believe) was never actually merged into the main sphinx_rtd_theme. I hoped we could keep it limping along, and we've needed to make a few changes to keep it working. But I agree that this is a maintenance issue.
In the near term, I'm fine with the change you have here, and I don't mind if you go ahead and merge it if you'd like. But before we go too far along with this, I wonder if we should revisit where documentation is hosted. Should we just move to hosting on readthedocs, for example, which I think might resolve these issues?
If you can find a way to support dynamic versioning (e.g., via the existing JavaScript or something else) with the new method without investing too much time, that could be ideal... but that could also be a rabbit hole, so I leave it up to you whether to pursue that.
Thanks again for your work on this, and I'm happy to talk more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a quick way to utilize the existing versions.json
, we're back to that being the only file that needs editing.
@billsacks are you planning to re-review or can this be merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all your work on this @jasonb5 ! I'm really happy that you found a way to use versions.json dynamically! This solution could be something we want to apply to other CESM documentation repos.
Test suite: n/a
Test baseline: n/a
Test namelist changes: n/a
Test status: n/a
Fixes n/a
User interface changes?: n/a
Update gh-pages html (Y/N)?: N