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.

Sign up for GitHub

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

Jump to bottom

Add void type #735

Merged
merged 2 commits into from
Oct 19, 2022
Merged

Add void type #735

merged 2 commits into from
Oct 19, 2022

Conversation

ussgarci
Copy link
Collaborator

@ussgarci ussgarci commented Oct 8, 2022 •
edited by byorgey
Loading

Add a new void type. Closes #665.

@byorgey
Copy link
Member

byorgey commented Oct 8, 2022

Looking good so far. You'll also need to add void to the parser for types. We'll also want to add a test or two to the test suite.

We might want to also mention void somewhere so players can discover it, for example, in the description of an ADT calculator.

@ussgarci
Copy link
Collaborator Author

ussgarci commented Oct 8, 2022

Thank you for the feedback and direction. I will review the parser code this evening.

@byorgey
Copy link
Member

byorgey commented Oct 9, 2022

@ussgarci I missed you in IRC, so just wanted to say: reading about megaparsec and parser combinators is great, don't let me stop you! But for this particular issue all you should have to do is figure out where the unit type is parsed, and then just copy-paste it for void. They should be handled in exactly the same way.

@ussgarci
Copy link
Collaborator Author

@byorgey Thank you for the tip.

@ussgarci
Copy link
Collaborator Author

Hi @byorgey , I've pushed an initial attempt at writing a test for the void type. Any feedback is appreciated.

byorgey
byorgey reviewed Oct 16, 2022
View reviewed changes
Copy link
Member

@byorgey byorgey left a comment

Choose a reason for hiding this comment

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

Looking good so far.

  • We could add some tests just to make sure that void is a valid type, for example, a test like valid "def f : void -> a = \x. undefined end" or something like that.
  • We could also put a test in TestPretty to make sure that the void type pretty-prints properly.

test/unit/TestVoid.hs Outdated
import Swarm.Language.Types
import Swarm.Language.Typecheck (isSimpleUType)

testVoid :: TestTree
Copy link
Member

Choose a reason for hiding this comment

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

This test is fine --- good job noticing that isSimpleUType had better return True for void, I wouldn't have thought of that. Fortunately, the way isSimpleUType is implemented, we get this for free.

test/unit/Main.hs Outdated
@@ -26,6 +26,7 @@ import TestLanguagePipeline (testLanguagePipeline)
import TestModel (testModel)
import TestNotification (testNotification)
import TestPretty (testPrettyConst)
import TestVoid (testVoid)
Copy link
Member

Choose a reason for hiding this comment

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

It's probably overkill to have a separate test module just for testing void. I would put the void tests in TestLanguagePipeline, perhaps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed. I've moved the test.

byorgey
byorgey approved these changes Oct 18, 2022
View reviewed changes
Copy link
Member

@byorgey byorgey left a comment

Choose a reason for hiding this comment

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

This looks great. We could update the ADT calculator description (or brainstorm other ways for the player to learn about void) in this issue, or it might be simpler to just merge this and split out in-game documentation of void into a separate issue.

@ussgarci
Copy link
Collaborator Author

Hi @byorgey merging this and splitting out the documentation of void sounds good to me.

@ussgarci ussgarci marked this pull request as ready for review October 19, 2022 00:35
@byorgey
Copy link
Member

byorgey commented Oct 19, 2022

@ussgarci OK, go ahead and add the merge me label. Do you want to create the new issue or should I?

@ussgarci
Copy link
Collaborator Author

@byorgey I can create the issue. Unless I'm mistaken, it appears I can't add a label to this PR.

@byorgey
Copy link
Member

byorgey commented Oct 19, 2022

Oh, right, I was confused and thought you were already added as a contributor.

@byorgey byorgey added the merge me Trigger the merge process of the Pull request. label Oct 19, 2022
@mergify mergify bot merged commit 758b3d0 into swarm-game:main Oct 19, 2022
@byorgey
Copy link
Member

byorgey commented Oct 19, 2022

Oops, forgot to edit the PR description before merging, oh well 😄

@byorgey
Copy link
Member

byorgey commented Oct 19, 2022

@ussgarci , congrats on having your first PR accepted to Swarm! We appreciate the contribution and we're really glad to welcome you as part of the community. In a minute we will send you an invite to the repo (see https://github.com/swarm-game/swarm/blob/main/CONTRIBUTING.md#i-have-push-access-to-the-swarm-repository-now-what ).

@ussgarci
Copy link
Collaborator Author

@byorgey Thank you for all your help! I'm happy to be a part of this community. 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@byorgey byorgey byorgey approved these changes

Assignees
No one assigned
Labels
merge me Trigger the merge process of the Pull request.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Void type
2 participants
@ussgarci @byorgey