-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Ensuring instruction labels are strings when provided to the Instruction constructor. #7671
Conversation
…ided to the Instruction constructor.
Pull Request Test Coverage Report for Build 2186209636
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking at this and making the PR - it looks good! I left a couple of minor comments, but nothing serious.
Please could you also add an "upgrade" release note for this - the instructions should be in the contributing guidelines. We just need to explain in one sentence that instruction labels will now be type-checked on creation.
Thanks for the feedback, @jakelishman! I've applied each of the requested changes. |
@jakelishman Do you think you'll have an opportunity to give this pull request another review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay! This looks good to me now, and thanks for pinging me so it didn't get lost.
We're getting some test failures here after this change, but I actually think that this PR is turning up buggy behaviour from existing code, and isn't the problem itself. I'll have a look into it, and see what's going on there. |
Thank you for the additional review and thank you for looking into the test failures! Feel free to let me know if you'd like any help. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue turned out to be that a particular couple of automated tests for gates were guessing on how to construct everything, and in some cases it was getting it wrong and passing an extra parameter that was getting put in the label
slot. This PR then made that a failure (as it should be), so we saw a test failure. I've pushed a commit that works around the issue (the automated instantiation is still pretty questionable), so this should be good to go now. Thanks!
Summary
Fixes #7667 by adding a type check to the
Instruction
constructor which will raise aTypeError
if thelabel
parameter is provided, but not of typestr
. Per the associated issue, this guarantees that an informative exception is raised for labels of an invalid type at instruction creation time rather than when the label is later processed byprint
ordraw
.Details and comments
Note that this pull request also adds a test for the type check that already existed for the
label
setter, but which was not under test coverage. I thought it prudent to add this test so that if our type checking rules change for labels in the future, we immediately know via a test failure that a change needs to be applied in both locations.