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

Fix: #1417 DefFrame to/from Quil with JSON values #1419

Merged

Conversation

caldwellshane
Copy link
Contributor

@caldwellshane caldwellshane commented Feb 9, 2022

Description

Closes #1417, #1418

Checklist

  • The PR targets the rc branch (not master).
  • Commit messages are prefixed with one of the prefixes outlined in the commit syntax checker (see pattern field).
  • The above description motivates these changes.
  • There is a unit test that covers these changes.
  • All new and existing tests pass locally and on the PR's checks.
  • Parameters and return values have type hints with PEP 484 syntax.
  • Functions and classes have useful Sphinx-style docstrings.
  • All code follows Black style and obeys flake8 conventions.
  • (New Feature) The docs have been updated accordingly.
  • (Bugfix) The associated issue is referenced above using auto-close keywords.
  • The changelog is updated, including author and PR number (@username, Test ignore #1234).

@caldwellshane caldwellshane requested a review from a team as a code owner February 9, 2022 06:20
@caldwellshane
Copy link
Contributor Author

caldwellshane commented Feb 9, 2022

@notmgsk This is ready for a look when you're up.

Copy link
Contributor

@dbanty dbanty left a comment

Choose a reason for hiding this comment

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

LGTM, though you might still want @notmgsk 's approval since he knows Quil much better than me.

assert (
fdef.out()
== r"""DEFFRAME My-Cool-Qubit "bananas":
DIRECTION: "go west"
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

@@ -60,7 +60,7 @@ def_circuit : "DEFCIRCUIT" name [ variables ] [ qubit_designators ] ":" indented
// | "DEFCIRCUIT" name [ variables ] qubit_designators ":" indented_instrs -> def_circuit_qubits

def_frame : "DEFFRAME" frame ( ":" frame_spec+ )?
frame_spec : _NEWLINE_TAB frame_attr ":" (expression | string )
frame_spec : _NEWLINE_TAB frame_attr ":" ( expression | string )
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so it already was string. I must've been looking at the wrong/old branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we were looking at master, but this was already done on rc. When I got here and saw it I assumed you'd done it. :)

@notmgsk
Copy link
Contributor

notmgsk commented Feb 9, 2022

Hella tight.

@notmgsk notmgsk merged commit f9608d8 into rigetti:rc Feb 9, 2022
@caldwellshane caldwellshane deleted the 1417-fix-defframe-tofrom-quil-with-json branch February 9, 2022 22:20
@rigetti-githubbot
Copy link

🎉 This PR is included in version 3.1.0-rc.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

dbanty pushed a commit that referenced this pull request Feb 14, 2022
* Fix: #1417 DefFrame to/from Quil with JSON values

* No code: Update changelog
@rigetti-githubbot
Copy link

🎉 This PR is included in version 3.2.0-rc.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@rigetti-githubbot
Copy link

🎉 This PR is included in version 3.2.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants