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

Bikeshedding is welcome! #22

Closed
aantron opened this issue Jul 17, 2018 · 41 comments
Closed

Bikeshedding is welcome! #22

aantron opened this issue Jul 17, 2018 · 41 comments
Labels

Comments

@aantron
Copy link
Owner

aantron commented Jul 17, 2018

Please do discuss any such issues. We want to make a pleasant API. We are keeping the Repromise version in 0.x.y for now to allow plenty of room for breakage :)

@ncthbrt
Copy link
Contributor

ncthbrt commented Jul 17, 2018

|. is the recommended form of chaining currently, so would advocate for fast pipe support. This has the downside of not being that pleasant in bucklescript/OCaml, but results in less indirection and better type inference on the reason side of things.

@aantron
Copy link
Owner Author

aantron commented Jul 17, 2018

@ncthbrt Is |. BuckleScript/Reason-only, though? If so, we would have to stick with |> for now, to keep Repromise native support. We'd probably have to upstream |. into OCaml, and then we can switch over the Repromise API to use |..

@ncthbrt
Copy link
Contributor

ncthbrt commented Jul 17, 2018

|. is Reason only.

Is |> a rewrite in native, or is it implemented as a function call?
Does a |> b become b(a) or is |> implemented as let (|>) = a f => f(a)

@aantron
Copy link
Owner Author

aantron commented Jul 17, 2018

|> is a function call, implemented in Pervasives: http://caml.inria.fr/pub/docs/manual-ocaml/libref/Pervasives.html#VAL(|>).

@aantron
Copy link
Owner Author

aantron commented Jul 17, 2018

So basically, |. is available on native, but only if using Reason. So it's not a matter of native vs. JS, but OCaml vs. Reason syntax.

@ncthbrt
Copy link
Contributor

ncthbrt commented Jul 17, 2018

|. Fast pipe

Pros

  • zero cost

Cons

  • not available using Ocaml syntax

|> Pipe

Pros

  • works everywhere

Cons

  • indirection
  • worse type inference due to indirection

@kennetpostigo
Copy link
Collaborator

kennetpostigo commented Jul 17, 2018

I would hope _new gets renamed to make since make is used nearly everywhere else. Also I think |> should be what is used, at least for now. Maybe |. support can come later, I don't think it's urgent.

@arnarthor
Copy link

I agree with make instead of new_ since it's pretty much standard for most libraries and bindings in Reason.

I'm torn with the piping, |. is highly encouraged in bucklescript because of no overhead but if it doesn't work in OCaml it's a decision of having nice syntactical support in OCaml or better runtime support in Reason + Bucklescript.

I'm not sure how important the runtime support is, but Hongbo has been pretty actively telling people to use fast pipe. For example all of Belt uses the fast pipe API design and even that is written in OCaml so the decision is pretty obvious in that case which is more important according to the maintainers.

@rizo
Copy link

rizo commented Jul 17, 2018

make all the things

I think make should become the new convention for constructor functions of a given module in Reason/OCaml. Examples: Person.make, Service.make, Repromise.make, etc.

Some libraries in OCaml use create, init or even v. make is better IMO.

There's only one way to pipe

Regarding the pipe operator. The less operators we have the better. I strongly prefer |> because it's a simple language-level construct. It's indirection cost is negligible. The type inference should be improved in the compiler. Introducing a new operator is a hack because this problem still exists for other similar operators.

By not forcing the main type to be the first argument, |> works better with partial application.

@rizo
Copy link

rizo commented Jul 17, 2018

Oh, and isn't ' better for names that collide with keywords? I think I prefer then' to then_.

@aantron
Copy link
Owner Author

aantron commented Jul 18, 2018

@rizo People coming from JS might be reluctant to use ', also it's more difficult to see (which could be an advantage or disadvantage). I prefer synonyms to avoid keywords. @thangngoc89 on Discord has suggested later, next. We could even resort to flatMap.

@thangngoc89
Copy link

@aantron @rizo and ' pretty much brokes the syntax highlighting

@aantron
Copy link
Owner Author

aantron commented Jul 18, 2018

Ok, since pretty much everyone here and in Discord wants new_ to become make, I opened an issue about it (#23). Leaving it available, in case someone wants to use it to get familiar with the code. Otherwise, I'll do it when Repromise is about to have some other breakage (which will be 0.6.0).

@thangngoc89
Copy link

@aantron I think flatMap is a better ideas than my useless bikeshedding about later and next. flatMap describes exactly what it does

@rizo
Copy link

rizo commented Jul 18, 2018

Elm uses and_then which is ok too.

@johnhaley81
Copy link
Contributor

I'd personally prefer flatMap & map with infix operators as >>= and <$> respectively.

🚪🏃‍♂️

@johnhaley81
Copy link
Contributor

A little trigger happy on the submit button

Although the |. operator seems to be preferred infix for piping right now with things like Belt, it feels really magical to me while |> is "just a function" so to me it seems easier to reason about and understand what's happening. I also prefer data last.

However, I also heavily subscribe to the "McDonald's Philosophy" of coding which states:

It's better to be consistently bad than inconsistently good.

So even if I think |> is superior, it seems like |. has the traction and I'd rather be consistent.

@aantron
Copy link
Owner Author

aantron commented Jul 19, 2018

I think the best thing for moving on |. is to try to get it into the regular OCaml, since it requires special support (as @rizo and @johnhaley81 pointed out). @bobzhang, do you know if there has already been an effort to upstream |.?

@aantron
Copy link
Owner Author

aantron commented Jul 19, 2018

...aaand new_ is now make, courtesy of @johnhaley81 in #24 (thanks!). Will release this in a couple days.

@rizo
Copy link

rizo commented Jul 19, 2018

@aantron I have to say that I’m against adding |. to OCaml. Specially because we already have |> which works extremely well with partial application. What I suggest is starting a discussion to address the inference issues associated with |> instead of adding new syntax.

@aantron
Copy link
Owner Author

aantron commented Jul 24, 2018

I'm thinking to rename then_ to andThen, Elm-style, as suggested by @rizo.

@arnarthor
Copy link

I think flatMap would be better, not because it's more descriptive, but because it's consistent with other libraries in the eco system (for example Belt).

Adding yet another term can be confusing, especially for newcomers, but also for those who are used to using flatMap in other libs.

@aantron
Copy link
Owner Author

aantron commented Jul 24, 2018

The issue I have with flatMap is that it if I am new to promises, it will make no sense to me how the name flatMap describes the operation that is done, nor what is the analogy between promises and options/results that justifies re-using the name. I realize you agree it's not descriptive, but I suggest the analogy doesn't hold that obviously, either. The types are similar, so it obviously holds on a type level, but I think the analogy is quite a bit weaker when thinking of semantics.

flatMap in option/result "looks," to a newcomer, relative to map, like just a nifty convenience function for collapsing ("flattening") options, while flatMap in promises is the fundamental function that allows scheduling more async computation in the future, whereas map schedules synchronous computation. That's why I find andThen appealing. It somehow captures the "additional async" semantics of the function.

Another concern I have is that when mixing options/results and promises, it might actually be beneficial to have two separate (and highly descriptive) names for chaining them, so that it's immediately clear which identifier refers to which datatype being manipulated. This might also be useful when modules are locally opened.

@rizo
Copy link

rizo commented Jul 24, 2018

The underlying abstract meaning of the operation is bind from the Monad interface. Scala uses flatMap but bind is more common in OCaml and Haskell. Even if repromise implements the Monad interface (with return, bind), I think having a more friendly alias like andThen is very useful.

@aantron
Copy link
Owner Author

aantron commented Jul 24, 2018

I think I'll indeed change it to andThen for now, at least for the next release :)

@aantron
Copy link
Owner Author

aantron commented Jul 24, 2018

Also, resolve is named that way by analogy with JS Promise.resolve, but I think resolved is a better name.

resolve seems like it was an attempt in JS to conflate returning a resolved promise with the action of resolving the final promise, in the usage return Promise.resolve(foo);

@rizo
Copy link

rizo commented Jul 24, 2018

I think resolved makes more sense indeed. What about Promise.of(42)?

Other options are:

  • Promise.const(42)
  • Promise.return(42)
  • Promise.pure(42)
  • Promise.with_value(42)

@arnarthor
Copy link

I like Promise.return(42) but resolved is good as well. Both are really descriptive of what is happening. I think the others mentioned @rizo are slightly unorthodox

@aantron
Copy link
Owner Author

aantron commented Jul 25, 2018

@rizo of is a keyword, so it won't work AFAIK.

@lpil
Copy link

lpil commented Jul 25, 2018

+1 to flatMap to be consistent with Belt.

@ivg
Copy link

ivg commented Jul 25, 2018

We should consider names qualified, i.e., in the way how they will look in the code. So here are alternatives (I personally hate camelCase and in general primitives that need more than one word in their name):

Promising (nothing new_)

  • Repromise.make ()
  • Repromise.init ()
  • Repromise.create ()
  • Repromise.fresh ()

Chaining (except then_)

  • promise |> Repromise.after (action)
  • promise |> Repromise.next (action)
  • Repromise.upon (promise, action) (for the flipped order of arguments)
  • promise |> Repromise.therefore (action)
  • promise |> Repromise.that (action)
  • promise |> Repromise.chain (action)

Fulfilling (it is resolved)

  • Repromise.fulfilled (value)
  • Repromise.resolved (value)
  • Repromise.finished (value)
  • Repromise.return (value)
  • Repromise.pure (value) (for mathematicians)
  • Repromise.now (value)

Not resolve definitely, as resolve is the name for a function that is the second element of a tuple returned by new_ (besides, did you consider making it abstract? It may save you a lot of hassle in the future)

Note, the order of suggestions is random.

@lpil
Copy link

lpil commented Jul 25, 2018

Given a qualified name I will think that flatMap is the clearest and most consistent name.

aantron added a commit that referenced this issue Jul 25, 2018
@aantron
Copy link
Owner Author

aantron commented Jul 25, 2018

  • Repromise.resolve is now Repromise.resolved (c7c622f). Repromise.Rejectable.reject also became Repromise.Rejectable.rejected.
  • I merged a PR to rename then_ to andThen (Rename Repromise.then_ to Repromise.andThen #27, thanks @jihchi!). We might have to do a poll or something to figure out whether to go with andThen or flatMap. However, since andThen is a unique identifier, it is straightforward to do a blind search-replace to change it to flatMap later.

@lpil
Copy link

lpil commented Jul 25, 2018

If we want a more specific time based name than flatMap do we want to do the same for map? It also applies the given function to the contained value after the promise has resolved, and also has a math-y name that describes the behaviour in relation of a rather abstract Functor type.

Thinking about the relationship between map and flatMap I don't see why flatMap is more suited to the name andThen, it says nothing about chaining on another async computation, which is what makes flatMap/andThen special.

@aantron
Copy link
Owner Author

aantron commented Jul 26, 2018

@lpil for me it's the and, it is more async after async.

@lpil
Copy link

lpil commented Jul 26, 2018

So map should be then? :)

@aantron
Copy link
Owner Author

aantron commented Jul 26, 2018

butThen :)

@rizo
Copy link

rizo commented Jul 26, 2018

Oh my... 😄 If I had to vote I'd just go for map and bind – the traditional names well understood in the community. I can see how andThen is more convenient, but please don't change map.

Aren't people supposed to use the ppx syntax anyway for binds?

@aantron
Copy link
Owner Author

aantron commented Jul 27, 2018

Ok, I released Repromise 0.6.0, with these renamings for now:

  • new_ to make
  • then_ to andThen
  • resolve to resolved
  • reject to rejected

Thanks for the discussion so far :)

@danny-andrews-snap
Copy link

danny-andrews-snap commented Oct 15, 2018

The underlying abstract meaning of the operation is bind from the Monad interface. Scala uses flatMap but bind is more common in OCaml and Haskell. Even if repromise implements the Monad interface (with return, bind), I think having a more friendly alias like andThen is very useful.

A few thoughts on this. First, aliases should be considered as a last resort because, although they are cheap from an implementor's POV, they are very expensive from a consumer's. At best, they require a special linting rule (if one even exists) to ensure your team uses one name over the other consistently, and at worst they cause unnecessary cognitive load when reading code that uses them interchangably. (I can't count the number of times I had to consult the docs to reassure myself that inject and reduce were equivalent in Ruby.) It's a HUGE PITA! (A concise argument against aliases can be found here.)

I think we ought to choose a name that is part of the monad interface, and avoid aliases altogether. I'm not sure if there is any such spec in Reason, but there exists an interface specification in JS that we could leverage: https://github.com/fantasyland/fantasy-land#monad. This spec suggests the use of chain in place of flatMap or andThen. I think this is a great choice since it is general enough to be applied to any type of monad (not just promises) and it is intuitive (you use it to chain operations).

Let me know what you guys think. :)

@aantron
Copy link
Owner Author

aantron commented Sep 10, 2019

Closing this as it seems settled for the time being, however bikeshedding is still welcome :)

@aantron aantron closed this as completed Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants