search

typelevel / algebra

Experimental project to lay out basic algebra type classes
https://typelevel.org/algebra/
Other
378 stars 69 forks source link

Would be sweet to use PRs #6

Closed johnynek closed 9 years ago

johnynek commented 9 years ago

I see the pushes, but not as PRs.

What if we start using PRs to be able to comment on code before it is merged?

non commented 9 years ago

You got me. I'm sorry for subverting good process.

non commented 9 years ago

I'll start using PRs again -- it definitely facilitates discussion. I think I've fallen prey to some bad habits.

johnynek commented 9 years ago

Cool. PS, if it wasn't the case before, 2.10 and 2.11 support would be the only thing we need for Algebird. Twitter finally got off 2.9. :)

I added some comments to the previous pushes. Did you get them? My little iOS github client let me do it, but maybe they all got lost. :/ I had several.

non commented 9 years ago

Yeah, I did get them (and I think I fixed all of them except the suggestion to make Sign a value class).

non commented 9 years ago

The reason I don't think that will work is that the pattern is like:

sealed abstract class Switch(val toInt: Int)
case object On extends Switch(1)
case object Off extends Switch(0)

In this case, there are never other allowed values. And I think the case object means On and Off will be boxed anyway. I can run some tests to be sure, but I think a value class might be a net loss here.

johnynek commented 9 years ago

Got it.

non commented 9 years ago

The first of many future PRs: #7

non commented 9 years ago

Trying to make up for lost time with #8 and #9 as well.

johnynek commented 9 years ago

Closing this one.