-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Expand docstrings for annotated flag on control()
and inverse()
#11749
Conversation
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 7869598198Details
💛 - Coveralls |
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.
Thanks for doing this it's a great improvement. I have one inline wording suggestion that applied to all the uses of annotated
on every standard library gate class so it's repeated for each inverse()
method.
Apply suggestions from Matt's code review Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Thanks for taking the time to write all the suggestions, I had to format them a bit to make lint happy, but I took it as a chance to double-check that the messages make sense for the corresponding gates. |
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.
Oops, look like I missed a few spots in my earlier review.
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
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 applying the updates
…11749) * Add annotated * Add returns * Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Apply suggestions from Matt's code review Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Format suggestions * Fix lint * Apply last round of suggestions Co-authored-by: Matthew Treinish <mtreinish@kortar.org> --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org> (cherry picked from commit 381a8d9)
…11749) (#11774) * Add annotated * Add returns * Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Apply suggestions from Matt's code review Co-authored-by: Matthew Treinish <mtreinish@kortar.org> * Format suggestions * Fix lint * Apply last round of suggestions Co-authored-by: Matthew Treinish <mtreinish@kortar.org> --------- Co-authored-by: Matthew Treinish <mtreinish@kortar.org> (cherry picked from commit 381a8d9) Co-authored-by: Elena Peña Tapia <57907331+ElePT@users.noreply.github.com>
Summary
Documents
annotated
argument forinverse()
in all gates in the standard library that provider their own implementation. I found thatcontrol()
was already documented, but I made sure the types were indicated in the signature instead of the args. definition to make @Cryoris happy (unfortunately there is still quite a lot of inconsistency documenting the types).Details and comments
Addresses #11707.
I only looked for gates in the standard library, if you know of any other affected gates, let me know and I can add them to the PR.