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

Lib/boxed #455

Merged
merged 3 commits into from
Jun 15, 2016
Merged

Lib/boxed #455

merged 3 commits into from
Jun 15, 2016

Conversation

kaeluka
Copy link
Contributor

@kaeluka kaeluka commented May 26, 2016

Some wrappers for boxed primitives.

Support a show method as a first step towards a general Show trait for printable objects.

@supercooldave
Copy link

I see a mix of camel case and snake case.

@kaeluka
Copy link
Contributor Author

kaeluka commented May 26, 2016

I'm using Uppercase names for boxed types, not CamelCase. The snake case is functions in the pre-existing String lib.

@supercooldave
Copy link

maxReal is CamelCase.

@kaeluka
Copy link
Contributor Author

kaeluka commented May 26, 2016

true!
I'll await the result of the ongoing slack discussion and act accordingly.

@supercooldave
Copy link

If the problems don't lie in your code, then your code should be acceptable.

@EliasC
Copy link
Contributor

EliasC commented May 26, 2016

Nice! Merging after Slack discussion is "done".

@@ -0,0 +1,8 @@
-- A boxed unit () value.
passive class Void {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this type be consistent and have the value method to return void?

Choose a reason for hiding this comment

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

Shouldn't this class be called Unit, as it seems that void and () (Unit) are two different things (FTW)?

@supercooldave
Copy link

Curious: what is the motivation behind this?
If we have need boxing, wouldn't it be better to do this under the hood (like in O'Caml or Haskell), especially as our intention, presumably, will be that boxed values are compared by value, not by reference.

@kaeluka
Copy link
Contributor Author

kaeluka commented May 30, 2016

Even if done under the hood, there'll be classes for those values. If there the "under the hood" stuff will come soon-ish, it might make sense to merge this into the under-the-hood-efforts.

@supercooldave
Copy link

If it were done under the hood, there'd probably be a single polymorphic class (perhaps called Box) that did the job of all of these classes.

@kaeluka
Copy link
Contributor Author

kaeluka commented May 30, 2016

If you had a Box class, you could not have a show method that converts the boxed primitive to string. Implementing support for a Show trait with such a method was my motivation to start this.

Edit: If I'm mistaken, then what @supercooldave says is true and this PR could pbly be rejected!

@supercooldave
Copy link

Thanks for providing motivation, albeit in comments not in PR.

@kaeluka kaeluka removed their assignment May 30, 2016
@kikofernandez
Copy link
Contributor

what is the status of this PR? I am kind of lost in the whole conversation

@kaeluka
Copy link
Contributor Author

kaeluka commented Jun 13, 2016

Good you ask. The last consensus (IIRC) was that it shouldn't go in as long as we're discussing a code convention for encore. We have stopped discussing the code convention. If people don't object, I'd close the PR tomorrow or late tonight to free up some cycles.

@supercooldave
Copy link

supercooldave commented Jun 13, 2016

Executive decision: change the name of the Void box to Unit box, add the corresponding value method, and it can be accepted.

@kaeluka
Copy link
Contributor Author

kaeluka commented Jun 14, 2016

@supercooldave: done!

@supercooldave supercooldave merged commit eec6b2f into parapluu:development Jun 15, 2016
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.

5 participants