Skip to content
This repository was archived by the owner on Jul 24, 2024. It is now read-only.

add switch_case to json decoder.py#637

Merged
kt474 merged 4 commits into
Qiskit:mainfrom
kevinsung:switch-case
Jul 20, 2023
Merged

add switch_case to json decoder.py#637
kt474 merged 4 commits into
Qiskit:mainfrom
kevinsung:switch-case

Conversation

@kevinsung
Copy link
Copy Markdown
Contributor

Summary

Adds switch_case to the JSON decoder. Not sure if/how this should be tested. Please advise.

Details and comments

@coveralls
Copy link
Copy Markdown

coveralls commented May 23, 2023

Pull Request Test Coverage Report for Build 5614902096

  • 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 52.012%

Totals Coverage Status
Change from base Build 5603235847: 0.0%
Covered Lines: 3154
Relevant Lines: 6064

💛 - Coveralls

@kt474 kt474 added this to the 0.6.1 milestone May 30, 2023
@merav-aharoni
Copy link
Copy Markdown
Collaborator

The file test/unit/transpiler/passes/scheduling/test_scheduler.py contains tests for various dynamic circuit constructs, such as for_loop, if_else, etc. So you can follow the examples there for switch.

@kevinsung
Copy link
Copy Markdown
Contributor Author

The file test/unit/transpiler/passes/scheduling/test_scheduler.py contains tests for various dynamic circuit constructs, such as for_loop, if_else, etc. So you can follow the examples there for switch.

Those tests don't seem to be testing the decoding. For example, if I comment out the lines below, those tests still pass.

"if_else": IfElseOp,
"while_loop": WhileLoopOp,
"for_loop": ForLoopOp,

@kt474 kt474 removed this from the 0.6.1 milestone Jun 9, 2023
Copy link
Copy Markdown
Contributor

@kt474 kt474 left a comment

Choose a reason for hiding this comment

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

I think your linter/editor reorganized the order of the dependencies, but the change itself LGTM

@nkanazawa1989 this adds on to your PR from #413

@kevinsung
Copy link
Copy Markdown
Contributor Author

I think your linter/editor reorganized the order of the dependencies, but the change itself LGTM

I did add an import statement and then used ruff to reorganize the imports. That's an improvement though, right? Do you want me to revert it?

@kt474 kt474 merged commit 16b0da2 into Qiskit:main Jul 20, 2023
@kevinsung kevinsung deleted the switch-case branch July 20, 2023 19:30
@kt474 kt474 added this to the 0.6.2 milestone Jul 23, 2023
@kt474 kt474 added the Changelog: New Feature Include in the Added section of the changelog label Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Changelog: New Feature Include in the Added section of the changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants