Skip to content
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

Fix a docstring rst link rendering within an inline literal #13567

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

eliarbel
Copy link
Contributor

Changing from this:

image

to this:

image

If someone knows if there is a way to properly render an rst link within an inline literal text, I'd love to know.

@eliarbel eliarbel requested a review from a team as a code owner December 16, 2024 10:51
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@coveralls
Copy link

Pull Request Test Coverage Report for Build 12351089492

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 24 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.03%) to 88.936%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 94.02%
crates/qasm2/src/lex.rs 5 91.23%
crates/qasm2/src/parse.rs 18 96.69%
Totals Coverage Status
Change from base Build 12296775949: -0.03%
Covered Lines: 79404
Relevant Lines: 89282

💛 - Coveralls

@@ -725,7 +725,7 @@ def operation_names(self):

@property
def instructions(self):
"""Get the list of tuples ``(:class:`~qiskit.circuit.Instruction`, (qargs))``
"""Get the list of tuples (:class:`~qiskit.circuit.Instruction`, (qargs))
Copy link
Contributor

@ElePT ElePT Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing this! What do you think of maintaining the code formatting for the rest of the tuple? It would look like this:

Screenshot 2024-12-16 at 13 23 06
Suggested change
"""Get the list of tuples (:class:`~qiskit.circuit.Instruction`, (qargs))
"""Get the list of tuples ``(``:class:`~qiskit.circuit.Instruction` ``, (qargs))``

It's not perfect but I find it a bit easier to read, probably personal choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshedding alert :-) before I opened the PR we had a little poll (of 3) here at the lab which included your suggestion and the applied fix was chosen unanimously. Personally I find it slightly cleaner visually.

Copy link
Contributor

@ElePT ElePT Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I am not to stand against unanimous votes. This change also looks good :)

@ElePT ElePT added documentation Something is not clear or an error documentation stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: None Do not include in changelog labels Dec 16, 2024
@ElePT ElePT added this pull request to the merge queue Dec 16, 2024
Merged via the queue into Qiskit:main with commit 40aa70c Dec 16, 2024
18 checks passed
mergify bot pushed a commit that referenced this pull request Dec 16, 2024
github-merge-queue bot pushed a commit that referenced this pull request Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog documentation Something is not clear or an error documentation stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants