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

Handle token_swapper impossible swap mapping with Error #971

Merged
merged 3 commits into from
Aug 22, 2023

Conversation

mtreinish
Copy link
Member

This commit fixes the handling of invalid mapping requests in the token_swapper rustworkx-core function and the graph_token_swapper function in rustworkx that uses it. Previously if an invalid mapping was requested the function would internally panic because it always assumed there was a path in the graph to fulfill the user requested mapping. However, because the rustworkx-core function didn't support error returns a breaking api change is needed to add a result return type.

This commit fixes the handling of invalid mapping requests in the
token_swapper rustworkx-core function and the graph_token_swapper
function in rustworkx that uses it. Previously if an invalid mapping was
requested the function would internally panic because it always assumed
there was a path in the graph to fulfill the user requested mapping. However,
because the rustworkx-core function didn't support error returns a
breaking api change is needed to add a result return type.
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, we need to break the API but it's a better solution than panicking

Copy link
Contributor

@enavarro51 enavarro51 left a comment

Choose a reason for hiding this comment

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

LGTM as well. Definitely needed.

@enavarro51 enavarro51 added the automerge Queue a approved PR for merging label Aug 22, 2023
@coveralls
Copy link

Pull Request Test Coverage Report for Build 5939995286

  • 37 of 43 (86.05%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 96.441%

Changes Missing Coverage Covered Lines Changed/Added Lines %
rustworkx-core/src/token_swapper.rs 26 32 81.25%
Totals Coverage Status
Change from base Build 5932667641: -0.03%
Covered Lines: 15392
Relevant Lines: 15960

💛 - Coveralls

@mergify mergify bot merged commit ffb1973 into Qiskit:main Aug 22, 2023
26 checks passed
@mtreinish mtreinish deleted the fix-panic-token-swap branch August 22, 2023 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Queue a approved PR for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants