Skip to content

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
@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