-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add missing docstrings for EquivalenceLibrary
#13291
Conversation
One or more of the following people are relevant to this code:
|
EquivalenceLibrary
EquivalenceLibrary
Pull Request Test Coverage Report for Build 11709092718Warning: 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.
Details
💛 - Coveralls |
crates/accelerate/src/equivalence.rs
Outdated
/// | ||
/// # Arguments: | ||
/// * `key`: The `Key` to look for. | ||
/// ## Arguments: |
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.
Is there a reason to go with H2 here?
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.
Nothing more than just me thinking H1 looks a bit too big here.
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.
Heh, understandable. Regardless, it looks like the Rust standard library uses H1 for a first level section, but it also looks like documenting arguments separately like we do in Python is actually not hip in Rust.
I would recommend that we just remove the Arguments
and Returns
sections on all of these methods altogether. To describe arguments that aren't self-explanatory, add a second paragraph after the summary sentence to describe the usage.
Here's what the Rust standard library uses as their convention: https://github.com/rust-lang/rfcs/blob/master/text/1574-more-api-documentation-conventions.md#appendix-a-full-conventions-text
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.
The Python docs look good. I left one comment recommending we use lowercase generic terms in cases where we don't want to make a code reference to a type. In general, prefer circuit
or :class:`.QuantumCircuit`
over QuantumCircuit
. This is a fairly standard practice, e.g. Rust gives this guidance in its standard library style guide. The rationale that comes to mind is that if we know the exact type in question, we might as well use a code reference.
As for the Rust docs, I'd recommend we drop all of the Args
and Returns
sections, and stick with just a summary sentence, followed by an optional paragraph that describes any usage details. It's just not standard practice in Rust to list out arguments like we see in Python docs. Instead, the Rust community favors a cohesive usage description of the function as a whole when arguments aren't obvious, along with an Examples
section, ideally for every method to provide usage examples. It's probably alright to skip the Examples
section for most of the methods you've got in this PR, since the usage is self-explanatory and this is not yet a public interface.
- Add docs links based on @kevinhartman's comments. - Add sphinx friendly labels in docstrings.
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.
LGTM, thanks for fixing this.
Summary
The following commits attempt to fix #13258 by adding all of the missing docstrings that were lost after #12585.
Details and comments
Add missing docstrings in the following methods:
EquivalenceLibrary
struct class.EquivalenceLibrary::get_graph
method.EquivalenceLibrary::py_keys
method.EquivalenceLibrary::py_node_index
method.EquivalenceLibrary::keys
method.