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

Revisit the Or datatype #11

Closed
stew opened this issue Jan 30, 2015 · 8 comments · Fixed by #36
Closed

Revisit the Or datatype #11

stew opened this issue Jan 30, 2015 · 8 comments · Fixed by #36

Comments

@stew
Copy link
Contributor

stew commented Jan 30, 2015

We now have a Or datatype in -data which is mostly lifted straight out of scalaz.

Right now it is

data Or a = LOr a | Ror a

We thought about -Or and Or- for constructors but the compiler seemed unhappy about. I don't think anyone loves the name LOr or ROr, but nobody seems to have a slam-dunk replacement. We're going to move on for now, but the discussion around this raised several questions we should revisit:

  1. What are good names for these constructors, some suggestions i've heard:
    Bad | Good
    \Or | Or/
    Dead | Alive
  2. do we want public constructors at all on these? Can we have just a catamorphism? Can we have smart constructors and extractors so that we can still pattern match?
  3. Do we want to tend not to just lift code out of scalaz, perhaps we would re-think some of the decisions made in scalaz and tend to have a better chance of improving things if we are writing everything new
@julien-truffaut
Copy link
Contributor

+1 for option 2

@rossabaker
Copy link
Member

  • -Or requires backticks. 👎
  • \Or requires backticks and a double . 👎
  • The L and R suffix on various types in scalaz-stream have never made me flinch.
  • Let's hold onto Eenie and Meenie in case we make an Or4 so we can have Miney and Mo. :)
  • Lifting from Scalaz bootstraps us faster and reduces the cognitive overhead for transitioning users, with the downside mentioned by @stew. I am neutral on this subject.

@ceedubs
Copy link
Contributor

ceedubs commented Feb 1, 2015

I'll throw in my 2 cents.

  1. I'd vote for something like LeftOr/RightOr, Lefty/ Righty, etc. These are only slightly more verbose than LOr/ROr, and I think they would be way easier for someone new to the library to recognize. I'd propose that we don't do something like Good/Bad. I say this because just last week I had two separate people tell me that they were thinking about using a disjunction but the left side wasn't going to be an error and they wanted to know if that was a bad idea. In my opinion that's a perfectly valid usage that I wouldn't want to discourage.
  2. I don't have strong feelings on this.
  3. Scalaz is a great library, but I don't think blindly lifting things from it is a great approach. I like the idea of questioning some design decisions, etc. I think it would be good to set up a list of principles/guidelines that direct decisions/implementations if possible.

@non
Copy link
Contributor

non commented Feb 1, 2015

  1. I think I like LeftOr and RightOr for now. Maybe we should try that and see how it feels?
  2. I like the idea of encouraging people to use factory constructors to end up with the "right" types. But I think leaving public constructors can help make pattern matching more efficient, so I guess I'm leaning in that direction. We could put the case classes inside the companion to make them slightly less accessible if we wanted.
  3. I think @travisbrown summed up my position pretty well in gitter: we should subject code from Scalaz to the same review process as anything else. In other words, we shouldn't blindly copy stuff over, but if there is a design that works we can definitely include it.

However I would like to be intentional about names, comments, and trying to keep things as modular as possible. I would encourage us to consider dropping methods that we aren't sure are needed -- we can easily add them back later. Let's not bring in the kitchen sink if we don't have to.

Anyway, just my 2¢.

@travisbrown
Copy link
Contributor

👍 to LeftOr and RightOr (and a stronger 👎 to Bad / Good or anything else that suggests an asymmetry beyond right-biasing).

I actually like the unpleasantness of -\/ and \/- in Scalaz, since it's a reminder that using them should require special justification. I can see the argument against them, though, and the redundancy / verbosity of LeftOr and RightOr could play kind of the same role (especially if they're in the companion object).

@julien-truffaut
Copy link
Contributor

I like LeftOr and RightOr but I would prefer they are defined in the companion object to limit their usage.

@rossabaker
Copy link
Member

I agree with the above. I'll rebuild this from a clean buffer as with LeftOr and RightOr and resubmit to for further conversation. It's a well-understood type, so it's a good one to help establish the principles and guidelines that @ceedubs mentions. Some specifics, while we're setting precedents:

  • Monomorphic this match vs. polymorphism. A few people have or recall benchmarks that suggest monomorphism is more efficient, but @mpilquist points out that polymorphism permits more specific return types where applicable.
  • Do we echo the Scalaz Functions and Instances trait structure, or do they all just go right onto the companion object?
  • Subclasses in the companion object, or alongside, or it depends?

@non
Copy link
Contributor

non commented Feb 1, 2015

Here's my feelings on this:

  • I would leave this to the author's discretion. I think there are too many variables here to make a general rule -- there are cases where more specific return types are important or essential, there may be cases where performance differs, etc. In this case I think matching is fine.
  • In algebra we separated out the functions (so they could be included in other companions too) but did not separate out functions. Previously my vague preference has been to leave things in the companion (except when hacking around implicit precedence). What do you all think?
  • I think it depends. In this case I would say we should put them in the companion to discourage use, but I can imagine other cases where this wouldn't be true. Maybe in the companion should be the default location, without being a requirement?

rossabaker added a commit to rossabaker/cats that referenced this issue Feb 2, 2015
Implements feedback from typelevel#11.  Specifically:
1. LOr and ROr move to `Or.LeftOr` and `Or.RightOr`.
2. Constructors are still public, but `left` and `right` preserved in
   `OrFunctions` to infer `Or`.  Use what you like.
3. Only the Scaladoc is ripped from Scalaz.

Also tries to adhere to the emerging consensus on typelevel#27.

Preliminar benchmarking showed monomorphism is faster than polymorphism,
and that implementing other operations using fold/bimap is immeasurable.
The second finding is dubious with respect to allocations.  If someone
can prove these guilty via benchmark, we can revisit.

I'd like to add tests after typelevel#29 happens.
@non non closed this as completed in #36 Feb 4, 2015
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 a pull request may close this issue.

6 participants