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

Call For API Review #2463

Closed
bobzhang opened this issue Jan 23, 2018 · 47 comments
Closed

Call For API Review #2463

bobzhang opened this issue Jan 23, 2018 · 47 comments
Milestone

Comments

@bobzhang
Copy link
Member

bobzhang commented Jan 23, 2018

It is not very polished yet, but we won't add any new features basically
The preview is here: https://bucklescript.github.io/bucklescript-tmp/api/Bs.html

TODO:

  • add some comments
  • more tests to avoid embarrassing bugs
  • address ocamldoc generated docs print Js.Vector instead of Js_Vector #2462
    Full coverage:
    • List
    • Array
    • [ ] Belt.Set.Dict
    • [ ] Belt.Set.Int
    • [ ] Belt_internalAVLset
    • [ ] Belt.MutableSet.Dict
    • [ ] Belt.MutableSet.Int
    • [ ] Belt_internalAVLtree
    • [ ] Belt.Map.Dict
    • [ ] Belt.Map.Int
    • [ ] Belt.MutableMap.Dict
    • [ ] Belt.MutableMap.Int

Leave hash based collection for later
Next:

  • We will add more modules like Option, String etc

Why:

  • significantly smaller size
  • better performance
  • JS friendly (exception-less, camlCase, consistent, t comes first )

Native story:

  • The API is designed to work on native
  • Conditional compilation after we upgrade to 4.06.1 (Next milestone)

========= some suggestions

  • General
    • Documentation format: needs reason syntax, dedicated doc page. Check immutable.js
    • Fewer things
    • Should have some clear interface for most of these maybe
    • Categorize functionalities: scanning, conversions, modifications, testing
  • Cmp
    • rename to compare/comparator
    • 'id -> 'identity, or some other names that's more descriptive. 'comparisonId?
    • 'a: give good description (same for 'id). Whats Bs.Cmp key/id?
    • What's make, and Make, and why would I getCmp (remove Make)
    • What's S (unfamiliar idiom. Can't expect js users to use this)
  • Hash
    • Check immutable.js hash docs, super good
    • eq -> equal
    • Same comments regarding S and and 'a and 'id
  • Array
    • Fewer things
    • Foreach: return true/false? Like immjs. Early stop - check some
    • Array.forall: sounds like foreach. every? All?
    • Where's exists/some?
    • Filter->keep
    • Filtermap/keepMap/mapKeep
    • Spotted some Js.undefined somewhere. Should be Js.nullable
    • Make vs init
    • makeMatrixExn: where's the no-exn one (remove)
    • shuffleDone: wtf? (Remove the done versions)
    • I also see a lack of labels, like Array.blitUnsafe has the signature 'a array -> int -> 'a array -> int -> int -> unit with lots of mysteriuous ints
    • Array.get and set are exception-raising, but not marked as such. I know that's probably because of array indexing sugar, but the sugar doesn't restrict the return type. At the very least I think it should be made VERY clear in the documentation that this function does not behave like the name implies it does. There's also no safe get and set functions
    • Copy, sub, fill, blit. Too many names for similar confusing promises
    • reduceFromTail is a pretty good and easy-to-understand name, but I think I'd prefer the less humpy reduceReverse instead.
    • truncateToLengthUnsafe (remove) - keep it for good reasons
    • Most identifiers are camleCase, but some are inexplicably snake_case. Is it for compat?
    • mapi -> mapWithIndex? (Maybe not, postponed)
    • forAll2, where's map2?
  • Queue
    • Is it fifo?
    • Is it mutative?
    • Can we not ship it for now (too many things, overwhelming, scares people)
    • Where's forEachi?
    • Create -> make
    • addDone, what is it
    • peekNull -> peekNullable
  • HashMap
    • Create -> make
    • 'a 'b 'id
    • What's this foo0 thing, foreach0 confusing with foreachi
    • Size/length, should be unified
    • hashmap vs map name confusion. mutableMap
    • Reset vs clear name, should be clear and reinitialize?
    • or why Bs.HashMap.getDict retuns a Bs_hash.t, not a dict
  • Sort
    • union: union src src1ofs src1len src2 src2ofs src2len dst dstofs cmp...
    • Inter -> intersect?
    • Diff -> difference
    • sortByCont?
    • binSearch -> binarySearch
    • Unclear link with Array
    • Also the sort module looks... very low level. I have no idea why anyone would use most of those
  • SortInt
    • elt...??
  • Stack
    • Why not just list
  • Range
    • forAllBy -> if that's the convention for passing a callback, we should use it everywhere, e.g. sortBy, findBy
  • Map
    • 'k 'v 'id should really be expanded
    • Minimum vs minNull
    • removeArray -> removeMany
    • t0
  • Bs.MapInt
    • has a fold function. most others have reduce I think
  • Bs.MapM
    • What's singleton (remove)
    • has a mapi function, but not a forEachi function, as I think Array has. I see no reason why this shouldn't be consistent
    • checkInvariant (remove)
    • setDone, etc. (remove)
    • In Bs.MapM, there are minKeyOpt and minKeyNull functions that return an option and null respectively. Then later there's a minimum functiion and a minNull function that does the same.
  • SetIntM
    • What's with the M (it's actually for Mutable)
  • List
  • @bs
  • The t comes first thing seems to be optimizing for the 1% use case of consuming BuckleScript from JS
  • I also don't understand the Done prefixed functions. Oh, I get the "done" functions now, and also not. So these things are mutable, but still support chaining by default. Yet the functions aren't in pipe form so you can't pipe them. That seems... odd. And also dangerous to have functions singifying that they are immutable by retuning a "new" collection, when in fact they don't. I'd like to have the mutability be much more apparent
  • I'm also not sure I like the Null suffix convention, or the need to have these at all since they can easily be derived from option
  • There are some randomly (or so it seems at least) abbreviated functions scattered around. Like Bs.Set has inter and cmp, but also singleton, which is no longer than either intersect or compare

==================================================================

  • Array
    • concatMany: list(array) please
      ==> see below, we can give List.concatMany : 'a list array -> 'a list
      ==> array is a more fundamental building block, also consistent with bs.splice
    • keepBy -> keep
    • every2, where's map2? ==> map2 was renamed to zipBy which seems better
  • List
    • concatMany: inconsistent with array.concatMany, will add flatten and concatMany
    • Length/size ==> will add both lenght/size to List and Array, length means linear which is not always true
  • Set
    • packDictData: ?? ==> will write a tutorial explain the principle
  • SetDict: ?? ==> same as above
  • MutableMap
    • Create is gone? ==> it is called empty, maybe a bad name, rename it into make?
    • Reset vs clear gone? ==> add clear
    • no forEachByKey function ==> forEach by default is key value pair
    • mapWithKey -> mapByKey ==>
      I feel mapWithKey looks better, map is mapping value, here we supply one more argument
  • MutableSet
    • Bs_SetM still present (leak to error reporting/type hints). Can we remove that
      will do
  • UnorderedMutableSet
    • Unsorted or unordered?
      In almost all other languages (except JS), unordered means unsorted..
    • MutableSet should be by default unsorted. Then have a SortedMutableSet (this one). Shorter too
    • Do the collections abide by insertion order? ==> No
  • General
    • Eq -> equal ==> let's keep the eq name : )
    • What about iterators ==> we can add it later, but I found it not very useful, for small amount of data, you would use array, for a large number of data, you would do fusion by hand
    • curried
      ========================================
@jchavarri
Copy link
Contributor

Thanks a lot for this @bobzhang it looks really great!

My only concern is something I brought up yesterday on Discord: the usage of [@bs] for callbacks in pure OCaml/Reason libraries.

The [@bs] attribute is far from ideal from a developer experience perspective, for several reasons:

  • It hurts code reuse. If I have an add(x,y) function and I want to pass add(10) to a List.map call, I have to do some extra noodling
  • It's not easy to explain to newcomers
  • It adds some overhead as all the definitions have to include the attribute
  • The errors are better thanks to super errors, but still not the most obvious thing to decypher
  • Ultimately, it breaks the consistency of an otherwise universal rule in OCaml / Reason: every function is unary.

The main benefits from what I understand are:

  • Bundle size: avoids having to require Curry (around 5 or 6 KB minified)
  • Performance: reduces the # of function calls
  • Output readability: produces f(x,y) in JS.

I certainly see the benefits, but I wonder if they outweigh the costs in developer experience. I am very interested to know your opinion, as well as others in the community.

Thanks again for your amazing work!!

@rafayepes
Copy link

Side comment regarding [@bs]. Another scenario where I found it useful was when using puppeteer's functions evaluate, which you can pass a callback that will be executed on the page you are manipulating. If that pages doesn't have Curry available in the global scope, I'll get a runtime error. Probably not a very common scenario, but I could only implement those puppeteer tests in Reason because I could remove those Curry calls using [@bs].

I do agree though with all the issues listed above by @jchavarri

@jchavarri
Copy link
Contributor

@rafayepes Oh definitely, sorry I wasn't clear. I'm referring exclusively to functions that are not built to interop in any way with external JS code. [@bs] is definitely a hard requirement to write externals with callbacks.

@yawaramin
Copy link
Contributor

Looks great Bob. Seems you're using some phantom type parameters in some types for safety? These will need to be documented carefully. Maybe a general overview of the technique to get everyone on the same page.

@OvermindDL1
Copy link
Contributor

The API is designed to work on native

Whooo! This is so important to help its adoption in my opinion. :-)

I'll try to look it over when I get time. :-)

@bobzhang
Copy link
Member Author

bobzhang commented Jan 24, 2018

per discussion we will provide both

forEach :  ('a -> uint) -> 'a t -> unit
forEachU (or forEach' or forEach_) : ('a -> unit [@bs]) -> 'a t -> unit

@yawaramin it will be documented

@TheSpyder
Copy link
Contributor

Any chance we can get modules with labelled functions? I use StringLabels and ListLabels in place of String and List in my code. A way to make labels the default would also be nice, or I could just keep my own stdlib wrapping project that swaps them 🤔

Happy to contribute if it would help ;)

@bobzhang
Copy link
Member Author

bobzhang commented Jan 25, 2018

instead of using cmp which is a bit cryptic, how about order? (compare can cause subtle bugs due to Pervasives.compare legacy)

bobzhang added a commit that referenced this issue Jan 25, 2018
@OvermindDL1
Copy link
Contributor

cmp is cryptic how so? That is the actual name of the 'compare' function in a variety of language so it definitely has precedence (though not necessarily in OCaml). order on the other hand sounds like some kind of mis-named sorting function to me...

@chenglou
Copy link
Member

comparator is still available

@Risto-Stevcev
Copy link

This looks awesome 😃
My vote is comparator function name and order or ordering type name

Will the reflection api (Js.Types) still be kept? if so can an instanceof function be added to it for objects?
It's really useful for dealing with callbacks that take untagged sums (where bs.unwrap can't be used). I think a jsConverter solution would be a lot easier to use for that though and preferable over reflection

@yawaramin
Copy link
Contributor

yawaramin commented Jan 28, 2018

A few more notes:

  • The Array and List modules need range functions, or maybe the Range module needs Range.list and Range.array functions, so that:
Range.list 0 5 = [0; 1; 2; 3; 4]
Range.array 0 5 = [|0; 1; 2; 3; 4|]
Range.list ~step:2 0 5 = [0; 2; 3]
Range.list ~inclusive:true 0 5 = [0; 1; 2; 3; 4; 5]
  • The collection types also need a groupBy function:
let even n = n mod 2 = 0
List.groupBy even (Range.list 0 5) = [
  true, [0; 2; 4];
  false, [1; 3]
]
  • Also I recommend putting e.g. HashMapInt, HashMapString inside HashMap, so: HashMap.Int, HashMap.String.

  • Also it would be great if the module types were captured in an Interface module so that people would have an easy target to hit if they are trying to (in the future) override or extend the default modules.

@bobzhang
Copy link
Member Author

bobzhang commented Jan 29, 2018

@yawaramin there is partition in your case. I added an indirection Bs.Map.Int.

I updated the docs page, a new round of review is appreciated.
Note it is quite lots of work, some enhancement (if not causing backwards incompatible changes can be delayed )

  • I did not add curried version of API yet
  • I feel cmp and eq is not too bad a name, given compare is already taken

@rauschma
Copy link

rauschma commented Jan 30, 2018

I second @TheSpyder’s wish for labeled parameters. I much prefer functions with labels. For example, compare the following two signatures:

      List.fold_left: (('a, 'b) => 'a, 'a, list('b)) => 'a
ListLabels.fold_left: (~f: ('a, 'b) => 'a, ~init: 'a, list('b)) => 'a

Labels are also recommended by the OCaml manual, whose suggestions I really like in this regard: https://caml.inria.fr/pub/docs/manual-ocaml/lablexamples.html#sec45

@rauschma
Copy link

Typo: getCmpIntenral()

@rauschma
Copy link

Lots of cool stuff in this library!

Iterators/enumarations would be nice. For example: https://ocaml-batteries-team.github.io/batteries-included/hdoc2/BatEnum.html

I’d also want streams, but I don’t know how they would fit into a world with iterators, Node.js, etc.

@jaredly
Copy link
Contributor

jaredly commented Jan 30, 2018

Node has streams!

@yawaramin
Copy link
Contributor

Indeed, and the Web API has iterators (not very nice ones though): https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols#The_iterator_protocol

@rauschma
Copy link

@jaredly I’d like streams + combinators or maybe just iterators + a stream that can optionally return an iterator.

@rauschma
Copy link

@yawaramin True. Maybe wrapping them would work. What I miss much in JS are combinators for iterators (zip, fold, etc.).

@bobzhang
Copy link
Member Author

bobzhang commented Jan 31, 2018

updated the new feedback, will address it later
@rauschma fixed the typo. We add labels occasisonaly like blit, I did not add it everywhere, since

  • If you follow the rule t comes first, callback comes last, the order is obvious
  • It is a bit too verbose

@rauschma
Copy link

@bobzhang OK. What’s the reason for inverting the order of parameter compared to OCaml’s stdlib? There, t always comes last, which is important for the pipe operator:

[4, 2, 1, 3, 5]
  |> List.map(x => x + 1)
  |> List.filter(x => x < 5)
  |> List.sort(compare);

I’d stick with the name filter (vs. keep) – that’s the name that everyone knows (OCaml, JavaScript).

@cullophid
Copy link

Having t as the first argument seems like a very bad idea. It would effectively break currying for all the Std functions.
You are essentially killing a very powerful feature of the language, just to make it look more like lodash.

@OvermindDL1
Copy link
Contributor

Agreed, bucklescript may compile to javascript, but if we wanted javascript's limitations then we would use javascript, not OCaml, and |> is crazy-useful. ^.^;

@yawaramin
Copy link
Contributor

@rauschma re: keep vs filter, I believe the keep functions behave differently than filter: they keep elements only as long as the given condition holds, and then discard all elements after that, for the entire collection. filter keeps those elements which pass the condition from the entire collection.

At least, that is the idiomatic meaning. I haven't tried these functions, so I can't guarantee that these obey that behaviour, but I believe they do--Bob can confirm.

Note that the existing OCaml stdlib functions are all still available since type 'a t = 'a list.

@Risto-Stevcev
Copy link

+1 on that, I think the main "target" in a function that would be used for piping should be the last argument in the function. For example, something that recently came up on discord a couple of times:

Js.Obj.assign(target, source) should be Js.Obj.assign(source, target) so you can use the pipe operator to merge multiple objects into a new one

Having a thread-first piping operator wouldn't fix this issue really, it still needs to be the last argument--because having it as the last argument also means you can make use of currying, and it also means that you can compose functions together without having to flip arguments around

@bobzhang
Copy link
Member Author

bobzhang commented Feb 1, 2018

I made an issue reasonml/reason#1452 explaining that why t comes first is better, note I am not alone, all janestreet libraries follow the same rules.
In a curried language, the last argument is not well defined, which argument is last one?

@yawaramin
Copy link
Contributor

To be fair, the Jane Street libraries use the 't first' convention along with labelled parameters to make life easier for people who want to keep piping. With t first and no labelled params, we can't pipe and partially apply any more, which disables some patterns that are almost universal in the ML world.

I do want to remind everyone though that this change will not affect anyone outside of the BS/Reason standard library. The existing OCaml libraries are still available and are more featureful in newer OCaml version 4.06 which is also on the roadmap for BuckleScript. No matter what design the BS/RE library uses, it will still be one option among several (albeit the primary option for many).

@bobzhang
Copy link
Member Author

bobzhang commented Feb 1, 2018

It is true that you can still use the existing stdlib. The new one is designed with performance in mind, I made an option for people who want to deploy efficient and smaller production code, but you don't have to (curry is essentially anti-perf)

@glennsl
Copy link
Contributor

glennsl commented Feb 1, 2018

all janestreet libraries follow the same rules.

That decision was made five years before the pipe operator was even added to the standard library, which makes the argument more of an appeal to tradition than an appeal to authority (which isn't a very good argument either).

I also don't get the performance argument. Not just because I don't know a single BuckleScript user (except its author, of course) who cares that much about performance, but you can still avoid partial application when needed with t last.

@bobzhang
Copy link
Member Author

bobzhang commented Feb 1, 2018

Hi, I hear your opinions, note t comes first does not mean we can not use pipe syntax (take GoLang, Elixir for example, they did a pretty good job).

Here are several reasons why t comes first, ordered by importance:

  • Last position is not well defined in a curried language, which argument is last one?
    Note it took me much more work to support bs.send.pipe compared with bs.send, in retrospect,
    I think it is a mistake to introduce bs.send.pipe
  • It caused inconsistency which can lead to subtle bugs.
    Take append x y and x |> append y for example, if we have a piped syntax for t comes first sugar, this would be good for all
  • It caused inconsistency between functional style code and imperative code.
    Please don't say that imperative is not important, it is equally as important as functional style, that's why I think OCaml is a practical language
  • It interacts worse with type inference (see the comment I made in another PR)
  • It is consistent with all other main stream languages
  • It encourages accidentally currying which is bad for performance

I would like to hear more about objective argument. Also I think pipe syntax has nothing to do with the order of t, that's why I made a proposal on Reason side, I would encourage you to chime in to comment on that issue reasonml/reason#1452

P.S. I used the janestreet codebase as an example not saying it is authority, but saying I am not just trying to be unique, I concur with their wisdom in this particular convention

@rauschma
Copy link

rauschma commented Feb 1, 2018

@bobzhang All of these arguments are valid. My complaint is that putting the positional parameter first doesn’t work well together with optional parameters. This is an interaction in rtop:

Reason # let f = (x, ~y=2, ~z=3) => (x, y, z);
Characters 18-36:
Warning 16: this optional argument cannot be erased.
let f: ('a, ~y: int=?, ~z: int=?) => ('a, int, int) = <fun>;
Reason # f(1);
- : (~y: int=?, ~z: int=?) => (int, int, int) = <fun>

Can that be fixed?

@chenglou
Copy link
Member

chenglou commented Feb 1, 2018

This can be fixed at the Reason side, if we do the |> + ? syntax change.

[1, 2, 3] |> List.map(?, foo) |> List.filter(?, bar)

It desugars to non-piped nested function syntax calls (the output doesn't change; |> is already specially compiled away in ocaml for perf). And since it's at the syntax level, no typing involved here and it's the semantic you think it is.

This wouldn't even be a breaking change; if we detect no ? after |> then we still delegate to the old |> semantics. Again, no output change, so no observable semantic difference even if you mix the two (unless you pass |> as a first-class function, but eh).

@bobzhang
Copy link
Member Author

bobzhang commented Feb 1, 2018

@rauschma in general, you don't use optional argument as last parameter, people used to add a unit argument

let f x ~y ?z () =
or 
let f x ?z ~y = 

Note this works worse with pipe
see different output for the v below

let f ?(x=1) z ?(y=2)  = 
   x + y + z   
(* let v = f 3    *)
let v = 3 |> f

@rauschma
Copy link

rauschma commented Feb 1, 2018

@bobzhang With pipe (and traditional OCaml style), you’d put all labeled parameters first, followed by all positional parameters.

I’m unhappy that you then need an additional unit parameter even though you already have a positional parameter:

/* OCaml style */
let f = (~y=2, ~z=3, x) => (x, y, z);

/* Positional first style */
let f = (x, ~y=2, ~z=3, ()) => (x, y, z);

That is, rules for optional parameters become more complicated than they are now.

@bobzhang
Copy link
Member Author

bobzhang commented Feb 1, 2018

Only optional parameter ?x need a unit, ~x is fine. A function that has all optional parameters is not that common in my experience

@rauschma
Copy link

rauschma commented Feb 1, 2018

But all labeled (apart from primary parameter) seems common. You’d evaluate partially more often than expected:

Reason # let f = (x, ~y=2, ~z) => (x, y, z);
let f: ('a, ~y: int=?, ~z: 'b) => ('a, int, 'b) = <fun>;
Reason # f(1, ~z=7);   /* not a function call */
- : (~y: int=?) => (int, int, int) = <fun>

You’d also need to add a unit parameter.

In other words: as soon as you have a single optional parameter, you need a positional parameter at the end.

@glennsl
Copy link
Contributor

glennsl commented Feb 1, 2018

@bobzhang Thanks for making some concrete arguments. I could still use some clarification though:

Last position is not well defined in a curried language, which argument is last one?

I don't understand what this means. The last argument isn't always the last argument? Are you referring to the last argument being polymorphic for example? Could you give an example?

It caused inconsistency which can lead to subtle bugs.
Take append x y and x |> append y for example, if we have a piped syntax for t comes first sugar, this would be good for all

Having multiple unlabelled arguments of the same type seems like bad style to me, and isn't a matter of t first or last, but of having multiple ts. Labelling one of them would fix both styles.

It caused inconsistency between functional style code and imperative code.
Please don't say that imperative is not important, it is equally as important as functional style, that's why I think OCaml is a practical language

Imperative is important. That's why I use OCaml rather than Elm or perhaps PureScript. But I don't see why these styles should be consistent. On the contrary, I want them to be inconsistent because they're different paradigms, and I'd like that to be really obvious.

We might be talking about different kinds of inconsistencies though. Giving examples would help.

It interacts worse with type inference (see the comment I made in another PR)

Noted. There might be minor issues in edge cases I've never encountered in practice. There are also edge cases with t first, however, although I've noted you saying the example given isn't "natural". I don't think it matters much either way though.

It is consistent with all other main stream languages

None of which have a pipe operator, and all of which (probably) uses other mechanisms for function chaining, such as having objects with methods that mutate internal object state.

It is possible to add a syntactic pipe operator to support piping with t first, as you've proposed, but that would be exclusive to Reason, and would be inconsistent with every existing OCaml library using the standard library pipe operator. We'd be moving further away from the OCaml ecosystem, which I think is a pretty significant decision and not summarized very well with "[being] consistent with all other mainstream languages".

It encourages accidentally currying which is bad for performance

What's "accidental" currying? Can you give an example?

@bsansouci
Copy link
Contributor

@glennsl I think @bobzhang is referring to having the last argument be optional, making it ambiguous which argument should be "the last one", see his example. Also the rest of the issue explains the potential type errors that we can avoid.

Totally guessing here. The "accidental currying" is, I think, that when you do 2 |> myFunc(1) you're using currying, which has compilation costs, type checking costs and runtime costs (I'm guessing it's more complicated to deal with than just calling the function?).

@rauschma I think what you're complaining about is about how optionals + autocurrying work together inside ocaml. This isn't something Reason can fix today unfortunately. But I think @bobzhang's proposal makes things less ambiguous, and with his syntax proposal here we'll get back the nice benefits of the pipe operator :)

@TheSpyder
Copy link
Contributor

TheSpyder commented Feb 6, 2018

I don't like t first because it doesn't read as well to me. I map a function over a list, not a list using a function. Which mainstream languages put t first? All of the ML derivatives I can think of put the function first, not the t, unless the map function is on the list object itself.

But again, this is where having labels lets you choose which order you want to use. I even switch to t first myself sometimes for readability (often when defining a multi-line function inline with the map call). They also avoid the need for a new pipe operator.

I'm going to throw in a new requirement - I'd like the stdlib to be as easy to extend as the ocaml stdlib. Regardless of whether we get labels or not, I'm likely to want a few custom things in all of my projects (e.g. infix monadic operators) so I expect to wrap this in a library that re-exports most of it with additions. That's also where I can add labels if they aren't in the default package.

This might seem like an obvious request, but the last work along these lines (bs-containers) wasn't extensible due to the use of "public": "none" in the bsconfig to hide the implementation files.

@glennsl
Copy link
Contributor

glennsl commented Feb 6, 2018

@bsansouci:

I think @bobzhang is referring to having the last argument be optional

I still don't see how that applies. That's a problem with t first, not t last, since t itself isn't optional.. t also shouldn't be polymorphic, so there's no ambiguity there either.

Also the rest of the issue explains the potential type errors that we can avoid.

Already noted, but really doesn't seem very significant. I've never had an issue with it, and if I do, it seems easy enough to fix. Inconvenient, yes, but a splinter in your toe is usually not worth chopping off your leg for :P. It's still very much a valid argument that should be considered though.

when you do 2 |> myFunc(1) you're using currying, which has compilation costs, type checking costs and runtime costs

The proposal you link just below fixes all that by making the pipe operator a pure syntactic transform.

"Accidental currying" could also refer to accidental partial application, which can occur with or without the pipe operator. The [@bs] calling convention would fix that, but seems like an entirely different discussion.

with his syntax proposal here we'll get back the nice benefits of the pipe operator

If what's implemented from that proposal is semantically (significantly) different from the standard library pipe operator, it also comes with some not so nice costs. More complexity, more concepts to learn, more cognitive load, familiar features behaving unexpectedly etc. The only proposal that seems like a clear win is to make the pipe operator, as it works today, a pure syntactic transform. And even that's arguable.

@chenglou
Copy link
Member

chenglou commented Feb 6, 2018

We'll make the pipe operator great enough. Give us a few more days. Stay tuned!

@Risto-Stevcev
Copy link

Risto-Stevcev commented Feb 6, 2018

Can @bobzhang or someone respond to @glennsl 's comments?
I'm also very confused about the arguments in favor of 't first'. Something like concrete examples of what 't first' solves that 't last' makes worse would be really helpful. I've read and re-read all the arguments so far and I still don't see anything that makes it better

@bobzhang bobzhang added this to the 2.2.0 milestone Feb 12, 2018
@bobzhang
Copy link
Member Author

@Risto-Stevcev I think I already made it clear, the different views are mostly philosophic. For example, in a curried language, you can not tell which argument is the last one by reading its types. I am not against sugars, but I don't want to pay for superficial abstractions, hence, I made a proposal to reason syntax to make both happy. I also think it is a good chance for reason syntax to fix some semantics of original ocaml design, introducing a syntax for pipe which could be better optimized, better error message, etc.

Thanks all for comments, let's ship a beta release and collect more feedback in practice

@mlms13
Copy link
Contributor

mlms13 commented Apr 18, 2018

Not sure where feedback is being collected in practice, but here's an anecdote from some code I was working on today:

Belt.Option.(
    Js.Dict.get(bundle##inputData, "property_status_list")
      |> flatMap(_, Js.Json.decodeArray)
      |> map(_, Js.Array.map(Js.Json.decodeString))
      |> flatMap(_, Util.OptArray.sequence)
      |> map(_, Array.to_list)
      |> map(_, String.concat(","))
      |> map(_, v => "/property-timelines?statuses" ++ v)
  );

I would consider this to not only be philosophic, but to be decidedly inconvenient in practice. As someone new to the ecosystem, it's also hard to remember that things that come from the OCaml stdlib (e.g. String.concat in the above snippet) and Js.Array (also in the above snippet) expect arguments in one order, while Belt utilities expect the opposite order.

I'm also unconvinced by the claim that lambdas look better in the final position. If they get too long, you can always pull them out into a let binding, and often times you already want to chain together named functions instead of lambdas.

Anyway, that's just my two cents.

@TheSpyder
Copy link
Contributor

Some of that is because Belt isn't finished; I assume it would eventually have it's own form of String.concat and Js.Array.

But in general, I agree. The arguments for t first - other than the obvious "it works like JS" which doesn't help with ocaml-style pipe chains - have all seemed theoretical to me. In practice t last is what I find works best, aside from a few cases where it's inconvenient and I can just swap the order because I'm using labels.

@bobzhang
Copy link
Member Author

@mlms13 are you aware of |. ?
@TheSpyder you can not really get consistent using t last , think about Array.get, Array.set and String.compare. We will settle on t first

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

No branches or pull requests