Skip to content

input tweaks and tests #99

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

Merged
merged 2 commits into from
May 21, 2025
Merged

Conversation

ajberdy
Copy link
Contributor

@ajberdy ajberdy commented May 15, 2025

Includes the following bug fixes:

  • Annotations for variables declared as inputs are now correctly passed to the IODeclaration node
  • Duration array inputs no longer attempt to convert the string "input" from float to duration

and adds tests

Copy link
Contributor

@braised-babbage braised-babbage left a comment

Choose a reason for hiding this comment

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

Looks good to me. One possible tweak suggested (hard to tell from here whether this matters).

@@ -416,7 +417,16 @@ def __init__(
array_base_type = base_type_instance.type_cls()

# Automatically handle Duration array.
if base_type is DurationVar and kwargs["init_expression"] is not None:
init = kwargs["init_expression"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering whether this should be kwargs.get("init_expression"). Basically we could have a KeyError now since previously the and would short circuit when base_type is DurationVar was false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, I can update that. I also want to make sure that it's clear what is owned by "init_expression" and what args is doing here

@ajberdy ajberdy requested a review from braised-babbage May 20, 2025 19:10
Copy link
Contributor

@braised-babbage braised-babbage left a comment

Choose a reason for hiding this comment

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

👍

@braised-babbage braised-babbage merged commit def54d5 into openqasm:main May 21, 2025
10 checks passed
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.

2 participants