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

Show missing type hints & inline class base class in API docs #13483

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Eric-Arellano
Copy link
Collaborator

The API docs were leaving off some important information:

  • For inlined classes, we weren't showing the parent class. This impacts when using .. autoclass:: on a module page; it was already fine for .. autosummary::.
  • Generally, we left off type hints for parameters without docstring, along with some return types like None. These type hints are useful.

To land this PR, I had to make a fix similar to #10772 to fix ambiguous type hints.

I also set up aliases for the extremely long types for EstimatorPubLike and SamplerPubLike. See Qiskit/qiskit-ibm-runtime#1877 for more context on that problem.

Copy link
Collaborator Author

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

See Qiskit/documentation#2377 for a preview of how this changes *the Git diff for the 1.3 dev docs.

Comment on lines +115 to +119
# Some type hints are too long to be understandable. So, we set up aliases to be used instead.
autodoc_type_aliases = {
"EstimatorPubLike": "EstimatorPubLike",
"SamplerPubLike": "SamplerPubLike",
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Feel free to disagree on this. The Runtime devs thought in Qiskit/qiskit-ibm-runtime#1877 that the full type is way too long to actually be understandable. I agree with them, and I think it's better to explain pubs in guides like https://docs.quantum.ibm.com/guides/primitive-input-output. But there's an argument for having the full type.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with the aliases, the full type is not really helpful.

@coveralls
Copy link

coveralls commented Nov 22, 2024

Pull Request Test Coverage Report for Build 11979624934

Details

  • 8 of 8 (100.0%) changed or added relevant lines in 7 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.01%) to 88.95%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 4 92.23%
Totals Coverage Status
Change from base Build 11977726745: 0.01%
Covered Lines: 79423
Relevant Lines: 89289

💛 - Coveralls

@Eric-Arellano Eric-Arellano marked this pull request as ready for review November 22, 2024 21:02
@Eric-Arellano Eric-Arellano requested a review from a team as a code owner November 22, 2024 21:02
@qiskit-bot
Copy link
Collaborator

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

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

Thanks Eric! I tried accessing the preview but I didn't see any in the PR you linked. Did I maybe miss something? It's been a while that I haven't checked a docs preview.

Comment on lines +115 to +119
# Some type hints are too long to be understandable. So, we set up aliases to be used instead.
autodoc_type_aliases = {
"EstimatorPubLike": "EstimatorPubLike",
"SamplerPubLike": "SamplerPubLike",
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with the aliases, the full type is not really helpful.

@Eric-Arellano
Copy link
Collaborator Author

I tried accessing the preview but I didn't see any in the PR you linked. Did I maybe miss something? It's been a while that I haven't checked a docs preview.

Oops, I meant to say how it changes *the Git diff to see a before and after.

@Eric-Arellano Eric-Arellano 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 Nov 25, 2024
Comment on lines -108 to -111
# Only add type hints from signature to description body if the parameter has documentation. The
# return type is always added to the description (if in the signature).
autodoc_typehints_description_target = "documented_params"

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a chance we are exposing parameters that were being kept private on purpose with this change? Else I wonder why this variable would have been set originally.

Copy link
Member

Choose a reason for hiding this comment

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

When we swapped from using the third-party sphinx-autodoc-typehints to the built-in sphinx.ext.autodoc support for this, I think I just set the options that caused the most similar behaviour to before (#9705).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there a chance we are exposing parameters that were being kept private on purpose with this change?

It wouldn't make sense to me to have certain parameters be private and others public. I suspect certain parameters aren't being documented simply due to oversight, or perhaps people thinking it's intuitive so it doesn't warrant docstring.

Also, the parameters are already in the docs as part of the signature. The only change this PR makes is showing their type hints.

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.

5 participants