-
Notifications
You must be signed in to change notification settings - Fork 39
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
Fixes #1523 #1561
Fixes #1523 #1561
Conversation
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.
Looks great! Thanks for taking this 😄 I have a small suggestion inline and two requests:
- Run
npm run format
and commit the changes as the CI lint check is failing - Add
"Fixes #1523"
to the PR description so it automatically closes the issue when we merge this
quint/test/runtime/compile.test.ts
Outdated
assertResultAsString('Set().setOfMaps(Set(3, 5)) == Set(Map())', 'true') | ||
assertResultAsString('Set().setOfMaps(Set()) == Set(Map())', 'true') | ||
assertResultAsString('Set(1, 2).setOfMaps(Set()) == Set()', 'true') |
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 makes it a bit easier to read and will also result in better error messages if it ever breaks (i.e. someone tries to remove one of the if
s you added). The way you wrote it will result in error messages like "expected true, got false" which is not as helpful as "expected Set(), got Set(Map())".
assertResultAsString('Set().setOfMaps(Set(3, 5)) == Set(Map())', 'true') | |
assertResultAsString('Set().setOfMaps(Set()) == Set(Map())', 'true') | |
assertResultAsString('Set(1, 2).setOfMaps(Set()) == Set()', 'true') | |
assertResultAsString('Set().setOfMaps(Set(3, 5))', 'Set(Map())') | |
assertResultAsString('Set().setOfMaps(Set())', 'Set(Map())') | |
assertResultAsString('Set(1, 2).setOfMaps(Set())', 'Set()') |
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.
Done! 😄
Thank you! For the "Fixes #1523", I think it needs to be on the description, not the title. I just edited your description to add it - see how "Fixes" gets a underscore and, if you hover over it, GitHub explains that this will close the appropriate issue. I'll go ahead and merge it. Thanks a lot for the contribution ✨ |
Empty domains and empty ranges in the setOfMaps operator now give Set(Map()) and Set() respectively, like TLC.
Fixes #1523