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.

Sign up for GitHub

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

Jump to bottom

Add method: has_node #1169

Merged
merged 4 commits into from
Apr 24, 2024
Merged

Add method: has_node #1169

merged 4 commits into from
Apr 24, 2024

Conversation

haoxins
Copy link
Contributor

@haoxins haoxins commented Apr 22, 2024 •
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.

@CLAassistant
Copy link

CLAassistant commented Apr 22, 2024 •
edited

CLA assistant check
All committers have signed the CLA.

mtreinish
mtreinish reviewed Apr 22, 2024
View reviewed changes
Copy link
Member

@mtreinish mtreinish left a 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

src/digraph.rs Outdated Show resolved Hide resolved
src/graph.rs Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Apr 22, 2024 •
edited

Pull Request Test Coverage Report for Build 8822336562

Details

  • 8 of 8 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 96.496%
Totals Coverage Status
Change from base Build 8821112146: 0.002%
Covered Lines: 17322
Relevant Lines: 17951
💛 - Coveralls

@haoxins
Add method: has_node
89ece5b 
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
@haoxins
Add release note
714191d
@haoxins haoxins requested a review from mtreinish April 22, 2024 14:38
mtreinish
mtreinish reviewed Apr 23, 2024
View reviewed changes
Copy link
Member

@mtreinish mtreinish left a 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.

tests/digraph/test_nodes.py Show resolved Hide resolved
@haoxins haoxins requested a review from mtreinish April 24, 2024 02:33
IvanIsCoding
IvanIsCoding approved these changes Apr 24, 2024
View reviewed changes
Copy link
Collaborator

@IvanIsCoding IvanIsCoding left a 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

@IvanIsCoding IvanIsCoding added the automerge Queue a approved PR for merging label Apr 24, 2024
@IvanIsCoding IvanIsCoding merged commit 7302c8b into Qiskit:main Apr 24, 2024
28 of 30 checks passed
@haoxins haoxins deleted the add_has_node branch April 25, 2024 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IvanIsCoding IvanIsCoding IvanIsCoding approved these changes

@mtreinish mtreinish Awaiting requested review from mtreinish

Assignees
No one assigned
Labels
automerge Queue a approved PR for merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@haoxins @CLAassistant @coveralls @mtreinish @IvanIsCoding