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

Feature: record structured output per shot, print json in transpile-to-quil #27

Merged
merged 12 commits into from
Apr 15, 2022

Conversation

nilslice
Copy link
Contributor

Could use some help w/ best placement for RecordedOutput and ideal visibility... it needs to be pub, and while it is used internally to qcs-sdk-qir, it is also used in an external to-be-published crate.

@nilslice nilslice requested review from dbanty and kalzoo April 10, 2022 19:57
src/transform/shot_count_block/pattern.rs Show resolved Hide resolved
src/transform/shot_count_block/pattern.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
tests/transpile-to-quil.md Show resolved Hide resolved
@nilslice
Copy link
Contributor Author

nilslice commented Apr 11, 2022

Note: this PR bumps MSRV to the latest stable: 1.60.0 and updates the edition to 2021.

No change to MSRV.

Copy link
Contributor

@kalzoo kalzoo left a comment

Choose a reason for hiding this comment

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

lgtm, the trait seems like a good way to solve this - although I wonder if a simple function (like you have commented out) would serve the same purposes with less complexity. That is, implementers of that trait only exist to then be formatted into a string, so maybe just a function that formats directly to string would do the job?

src/output/mod.rs Outdated Show resolved Hide resolved
src/output/mod.rs Outdated Show resolved Hide resolved
src/output/debug.rs Outdated Show resolved Hide resolved
src/output/mod.rs Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
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 🎸 . I left a couple comments & questions but certainly nothing blocking.

src/output/debug.rs Show resolved Hide resolved
src/output/debug.rs Outdated Show resolved Hide resolved
src/output/debug.rs Show resolved Hide resolved
src/output/mod.rs Outdated Show resolved Hide resolved
src/transform/shot_count_block/pattern.rs Show resolved Hide resolved
src/transform/shot_count_block/pattern.rs Show resolved Hide resolved
src/output/debug.rs Outdated Show resolved Hide resolved
src/output/mod.rs Show resolved Hide resolved
src/transform/shot_count_block/pattern.rs Outdated Show resolved Hide resolved
@nilslice nilslice merged commit 406c43e into main Apr 15, 2022
@nilslice nilslice deleted the sm/qir-record-output branch April 16, 2022 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants