-
Notifications
You must be signed in to change notification settings - Fork 60
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
Change source code links to go to GitHub #170
Change source code links to go to GitHub #170
Conversation
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.
Let's double check the artifact in CI works correctly once CI runs, but otherwise lgtm. Thanks!
@melechlapson What version of black do you have installed locally - here its still using the 0.22 release (needs to be updated but that should be a different PR as it will most likely impact other files). |
I believe I'm using 23.12.1 |
@woodsp-ibm I made the appropriate changes now |
Pull Request Test Coverage Report for Build 8756961707Details
💛 - Coveralls |
@Eric-Arellano Just a question around this
If you look are what is published for algorithms clicking on method does take you to the start of method, and as it was the docs were self contained, as they contained formatted source and did not rely on access to the github repo. I guess this seems fine... though I can imagine, since it does not have the formatted source as parts of the docs that the links could end up being incorrect, E.g we often backport code fixes ahead of releasing it an update - as such things may be changed so that while it was correct with the stable version at the time it was publishing some things may no longer be. Maybe this keeps things more consistent with elsewhere - but I must admit otherwise I am missing what the improvement really is - is the intent to do the app repos likewise. I did see perhaps it may link more directly to other aspects that I did not try out, such as enums from what I read |
Ah, true, the PR description is misleading for this repo and repos that host the Sphinx docs directly. With those projects, you're correct that The main reason you may want to land this PR is if you prefer the links to go to GitHub, rather than the code file Sphinx generates for you. GitHub is nice because it makes it easier to navigate to other files in the repo, and it has for example the right table of contents to help you jump around the file. The downside of this PR is more complex code. So I'm definitely okay with either direction you want to take.
You'll have a somewhat similar problem with viewcode when backporting code changes but not redeploying the docs. The code with viewcode will be out of date. With linkcode, it will take you to GitHub with the updated code, but it might take you to the wrong lines. Also that updated code won't yet be live for end users until the patch release, so the viewcode behavior is probably more correct. |
Apologies for the inaccuracies in the description I've updated it to be more precise |
We do backport/publish doc fixes outside of the bug-fix releases - in this case, without care its possible that other code fixes, not yet released would be pushed out into the docs making them not quite correct that way - so care needs to be done when doing this with viewcode too. This does seem to be a bit more complex and not self-contained in the way the way docs and formatted source are published with viewcode - but as you say you do end up in github where you have better navigation of the source.
No worries, just me trying to better understand things. @ElePT Any thoughts. I think I am ok with this and seeing how things go with the docs like this instead. |
Fwiw, I'm totally okay with either outcome. I do think GitHub links are nicer, but it does indeed make the config more complex. Whereas for Qiskit, Runtime, and Provider, this PR was a huge upgrade because it meant we could use precise source code links unlike before only going to the overall file. |
@melechlapson After reflection, since the docs here already do link to the source, and as the apps do their docs this same way, to have all these consistent I think it's perhaps both simpler and easier maintenance-wise overall to stick with what is already done with the docs here. But thanks for doing this and showing us another way - maybe if things change in the future it's something we might reconsider, but for now, in sticking with how the docs are already done here, I'll just close this off. Thanks for your understanding. |
Summary
This would switch out the
sphinx.ext.viewcode
extension for thesphinx.ext.linkcode
extension which allows source links in the API documentation to be linked to the specific lines of code in GitHub, instead of the sphinx-generated file. This was tested successfully in Qiskit/qiskit_sphinx_theme#589 and we have now implemented it in the qiskit, runtime, provider, and qiskit-aer repos.Example links generated in this PR build:
a class definition https://github.com/qiskit-community/qiskit-algorithms/blob/main/qiskit_algorithms/eigensolvers/eigensolver.py#L27-L67
a method https://github.com/qiskit-community/qiskit-algorithms/blob/main/qiskit_algorithms/amplitude_estimators/amplitude_estimator.py#L26-L34
Details and comments
Since some references in the API documentation actually refer to methods from the inherited qiskit classes, it was necessary to add a check at the end to change the base url if the method was from the qiskit class and link to the qiskit repo.