Change source code links to go to GitHub

melechlapson commented 5 months ago

Summary

This would switch out the sphinx.ext.viewcode extension for the sphinx.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 https://github.com/Qiskit/qiskit_sphinx_theme/pull/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.

CLAassistant commented 5 months ago

All committers have signed the CLA.

woodsp-ibm commented 5 months ago

@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).

melechlapson commented 5 months ago

@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

melechlapson commented 5 months ago

@woodsp-ibm I made the appropriate changes now

coveralls commented 5 months ago

Pull Request Test Coverage Report for Build 8756961707

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 90.653%

Totals
Change from base Build 8634991708:	0.0%
Covered Lines:	6488
Relevant Lines:	7157

💛 - Coveralls

woodsp-ibm commented 5 months ago

@Eric-Arellano Just a question around this

switching out the sphinx.ext.viewcode for the sphinx.ext.linkcode extension which allows our GitHub links in the documentation to be linked to the specific lines of code, not just the file.

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

Eric-Arellano commented 5 months ago

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 viewcode already takes you to the specific code in question. This PR does not make things any more "precise". That was only true for docs projects hosted on docs.quantum.ibm.com.

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.

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.

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.

melechlapson commented 5 months ago

Apologies for the inaccuracies in the description I've updated it to be more precise

woodsp-ibm commented 5 months ago

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.

Apologies for the inaccuracies in the description I've updated it to be more precise

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.

Eric-Arellano commented 5 months ago

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.

woodsp-ibm commented 5 months ago

@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.

qiskit-community / qiskit-algorithms