-
Notifications
You must be signed in to change notification settings - Fork 145
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 method: has_node #1169
Add method: has_node #1169
Conversation
haoxins
commented
Apr 22, 2024
•
edited
Loading
edited
- I ran rustfmt locally
- I have added the tests to cover my changes.
- I have updated the documentation accordingly.
- I have read the CONTRIBUTING document.
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, overall this is a nice addition! I left a small inline comment to simplify the code slightly. But besides that could you add a unit test that validates the case when this new method returns false? This also is missing the type annotation stubs and it'll fail the ci job checking those. The process for adding them is documented here: https://github.com/Qiskit/rustworkx/blob/main/CONTRIBUTING.md#type-annotations
Also if you don't mind could you add a release note to document the new API. The process for doing this is documented here: https://github.com/Qiskit/rustworkx/blob/main/CONTRIBUTING.md#release-notes
Pull Request Test Coverage Report for Build 8822336562Details
💛 - Coveralls |
clippy Add tests Update src/digraph.rs Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Update src/graph.rs Co-authored-by: Matthew Treinish <mtreinish@kortar.org> Add type annotations
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 the quick update! This looks ready to go to me now, the only thing I think missing is test coverage for PyGraph
. Once that's there I think this is good to merge.
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 adding the tests