qiskit
qiskit copied to clipboard
Qiskit
[wip] Use `sphinx.ext.linkcode` for more precise source code links
Summary
This addresses https://github.com/Qiskit/documentation/issues/517 by 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. This was tested successfully in https://github.com/Qiskit/qiskit_sphinx_theme/pull/589 so now we want to implement it in the qiskit, runtime, and provider repos.
Example links generated in this PR build:
- a class definition https://github.com/Qiskit/qiskit/tree/main/qiskit/circuit/controlflow/break_loop.py#L21-L47
- a method https://github.com/Qiskit/qiskit/tree/main/qiskit/circuit/instruction.py#L229-L269
- a function https://github.com/Qiskit/qiskit/tree/main/qiskit/utils/deprecation.py#L104-L204
The API generation script at qiskit/documentation already knows how to handle sphinx.ext.linkcode.
Determining the GitHub branch
We need to detect when to use main vs stable branches like stable/1.0, particularly for stable docs builds with GitHub Actions that get used by qiskit/documentation to deploy the docs. PR and local builds are less critical because the docs don't get published and are only for previews by code authors.
We do this with GitHub Actions environment variables. This logic was tested with GitHub Actions in qiskit/qiskit-sphinx-theme:
- branch build: https://github.com/Qiskit/qiskit_sphinx_theme/actions/runs/7995323122/job/21835383391#step:13:34
- tag build: https://github.com/Qiskit/qiskit_sphinx_theme/actions/runs/7995394559/job/21835625079#step:13:34
PR builds and the merge queue use Azure Pipelines. To keep this infrastructure simpler, we simply default to main in PR builds, rather than adding Azure support. This means the URL will go to the wrong branch in docs previews for PRs against stable branches, although the stable branch build will work correctly. I don't expect this to matter much because PR builds are solely for docs previews; I'm skeptical code authors will be opening up the [source] link to double check the line numbers are exactly correct, since that line number calculation is 100% automated.
Alternative considered
Rather than environment variables, we considered instead setting a value is_dev in conf.py. But we didn't like that it required manual changes, which we can forget to do. It also would behave improperly when making the commit on main that triggers the creation of the first rc1 for a new minor series; the newly created branch would still have is_dev=True because we can't set is_dev=False on the main branch . Instead, @mtreinish proposed this environment variables approach.
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.
Thank you for opening a new pull request.
Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.
While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.
One or more of the the following people are requested to review this:
- @Eric-Arellano
- @javabster
@Qiskit/terra-core
Pull Request Test Coverage Report for Build 8570280193
Warning: This coverage report may be inaccurate.
This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
- For more information on this, see Tracking coverage changes with pull request builds.
- To avoid this issue with future PRs, see these Recommended CI Configurations.
- For a quick fix, rebase this PR at GitHub. Your next report should be accurate.
Details
- 0 of 0 changed or added relevant lines in 0 files are covered.
- 48 unchanged lines in 11 files lost coverage.
- Overall coverage decreased (-0.002%) to 89.367%
| Files with Coverage Reduction | New Missed Lines | % |
|---|---|---|
| qiskit/circuit/store.py | 1 | 94.29% |
| qiskit/circuit/parameter.py | 1 | 98.39% |
| qiskit/circuit/operation.py | 3 | 82.35% |
| qiskit/circuit/bit.py | 3 | 91.49% |
| qiskit/circuit/instruction.py | 4 | 95.91% |
| qiskit/circuit/controlflow/if_else.py | 4 | 97.21% |
| crates/qasm2/src/lex.rs | 5 | 91.86% |
| qiskit/circuit/instructionset.py | 6 | 86.15% |
| qiskit/circuit/delay.py | 7 | 71.19% |
| qiskit/circuit/annotated_operation.py | 7 | 91.21% |
| <!-- | Total: | 48 |
| Totals | |
|---|---|
| Change from base Build 8559498936: | -0.002% |
| Covered Lines: | 60293 |
| Relevant Lines: | 67467 |
💛 - Coveralls
Thank you for opening a new pull request.
Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.
While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.
One or more of the the following people are requested to review this:
- @Eric-Arellano
- @javabster
@Qiskit/terra-core
This is now generating correct links in the PR preview, e.g. https://github.com/Qiskit/qiskit/blob/main/qiskit/circuit/gate.py#L169-L226. Should be ready to merge
