-
Notifications
You must be signed in to change notification settings - Fork 38
Feature 2771 subprojects #2942
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
Feature 2771 subprojects #2942
Conversation
…ontributor's Guide
…nt is not hidden behind the sticky header
…ected element is not hidden behind the sticky header
…(disappearing into the hamburger menu)
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 noticed the URLs for METplus-Training include the feature branch, which will go away when the PR is merged. This should be fixed before merging.
I apologize if this was already discussed and I missed it, but I am concerned that using hard-coded URLs to link to the latest
documentation for each sub-project will cause confusion. For example, users could be looking at the develop or feature branch version of the docs, then the link to another sub-project would take them to the latest version, which corresponds to an incompatible version of the software.
I tried to do some testing but I wasn't able to come up with a good solution to handle this. An RTD best practices page mentions to use intersphinx for links to other Sphinx projects, and this Intersphinx page has a note that says:
If you are using Read the Docs’ subprojects, you also need to enable the Intersphinx extension on each of the subprojects. For each subproject, you need to add the main project and all the other subprojects to intersphinx_mapping.
However, I wasn't able to get this to work and according to this issue it appears that intersphinx links aren't supported in toctrees.
I also tried to use relative paths for the links to try to preserve the doc version, but realized that this won't work because the branch name for a given page does not always have the same branch name for other projects.
docs/index.rst
Outdated
|
||
Users_Guide/index | ||
Verification_Datasets/index | ||
METplus Tutorial <https://metplus-training.readthedocs.io/en/feature_metplus2771_subprojects/Tutorial/index.html> |
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.
These URLs should not point to the feature branch
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.
Thank you for your thorough review and for catching this @georgemccabe! Thank you also for making us aware of your concerns for using hard-coded URLs to link to the latest
documentation for each sub-project.
Fortunately, the reference to feature_metplus2771_subprojects is only in the links to METplus-Training. Unfortunately, it already was merged in with MET, METcalcpy, METdataio, METplotpy, and METviewer.
@bikegeek @JohnHalleyGotway @michelleharrold @georgemccabe I can either:
- back out these changes for further discussion, moving this issue to the RC1 release so that we can proceed with the beta2 releases
- update the links directly in the develop branch, replacing
feature_metplus2771_subprojects
withlatest
and we can discuss any changes/modifications later - wait on the beta2 releases until we have a solution we're happy with
My preference is option 2. What are your thoughts?
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 vote option 2 as well. I think it would be good to get this in place so we and other users can use it and suggest improvements. It could be that my concerns are not a big issue after all
@georgemccabe I have updated the link to remove the reference to the feature branch and replace it with "latest". Could you please re-review when you get a chance? |
Pull Request Testing
Reviewed modified Table of Contents in Read the Docs here.
Review modified Table of Contents in Read the Docs here.
Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]
Do these changes include sufficient testing updates? [Yes]
Will this PR result in changes to the test suite? [No]
If yes, describe the new output and/or changes to the existing output:
Do these changes introduce new SonarQube findings? [No]
If yes, please describe:
Please complete this pull request review by [Before the beta2 release].
Pull Request Checklist
See the METplus Workflow for details.
Select: Reviewer(s) and Development issue
Select: Milestone as the version that will include these changes
Select: Coordinated METplus-X.Y Support project for bugfix releases or METplus-Wrappers-X.Y.Z Development project for official releases