-
Notifications
You must be signed in to change notification settings - Fork 38
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
Prepare to release sum type support in verify
#1274
Conversation
Use current conventions for operator names
Ensure we can handle this construct in verification, and it also clarifies the meaning of the operator tweaked.
a6df098
to
f0f0c3e
Compare
verify
verify
verify
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.
LGTM, only one non-blocking point
# We only want to print additional info to annotate failing results | ||
if [[ $succeeded == false ]]; then |
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.
I'd rather not have this if and always keep the explanations updated. If something is succeeding, then there should be no explanation, and a new failing thing should prompt us to "investigate" enough to assign a new explanation to the failing cell. Otherwise, we might fall back to old explanations that are not the real problem anymore.
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.
This conditional ensures that will only be an explanation if something fails. If it success, we don't print the explanation. My thinking was that will result in a bigger diff and a better chance of someone noticing that this needs to be updated. With the current logic, the explanation is printed whether or not the run succeeds, which I think makes it less likely for the required change to be noted.
It's exactly your concern that motivated this change :)
But I think we ought to rework this script entirely before long. So if you don't object hard here, I'll leave it as is and either adjust in a followup or when we rework this.
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.
LGTM!
@@ -12,10 +12,14 @@ | |||
* Gabriela Moreira, Informal Systems, 2022-2023 | |||
*/ | |||
module tictactoe { | |||
type Player = X | O |
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.
That's nice :)
Thanks for the reviews! |
This PR includes the updates we need prior to cutting a release that brings support of sum types into the
verify
command.CHANGELOG.md
for any new functionalityREADME.md
updated for any listed functionality