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

Prepare to release sum type support in verify #1274

Merged
merged 8 commits into from
Dec 4, 2023

Conversation

shonfeder
Copy link
Contributor

@shonfeder shonfeder commented Nov 28, 2023

This PR includes the updates we need prior to cutting a release that brings support of sum types into the verify command.

  • Updates to examples to ensure we are testing some edge cases
  • Updates to the examples dashboard and runner script to reflect current abilities
  • Updating the apalache version pinned to the latest release
  • Updating the feature table on the readme

  • Tests added for any new code
  • Documentation added for any new functionality
  • Entries added to the respective CHANGELOG.md for any new functionality
  • Feature table on README.md updated for any listed functionality

Shon Feder added 7 commits December 1, 2023 16:30
Use current conventions for operator names
Ensure we can handle this construct in verification, and it also
clarifies the meaning of the operator tweaked.
@shonfeder shonfeder force-pushed the 1034/use-sum-types-in-tictactoe branch from a6df098 to f0f0c3e Compare December 1, 2023 23:29
@shonfeder shonfeder requested a review from bugarela December 1, 2023 23:31
@shonfeder shonfeder changed the title WIP: Use sum types for tictactoe example Use sum types for tictactoe example Dec 1, 2023
@shonfeder shonfeder changed the title Use sum types for tictactoe example Prepare to release sun type support in verify Dec 2, 2023
@shonfeder shonfeder changed the title Prepare to release sun type support in verify Prepare to release sum type support in verify Dec 4, 2023
Copy link
Collaborator

@bugarela bugarela left a 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

Comment on lines +53 to +54
# We only want to print additional info to annotate failing results
if [[ $succeeded == false ]]; then
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@konnov konnov left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

That's nice :)

@shonfeder
Copy link
Contributor Author

Thanks for the reviews!

@shonfeder shonfeder merged commit f52fd2f into main Dec 4, 2023
15 checks passed
@shonfeder shonfeder deleted the 1034/use-sum-types-in-tictactoe branch December 4, 2023 12:23
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.

3 participants