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

Syntax for piping into the first argument #2342

Closed
bobzhang opened this issue Dec 8, 2017 · 22 comments
Closed

Syntax for piping into the first argument #2342

bobzhang opened this issue Dec 8, 2017 · 22 comments

Comments

@bobzhang
Copy link
Member

bobzhang commented Dec 8, 2017

relevant reasonml/reason#1452

data #. add  x y 
(* translated --> *)
add data x y 
(data : M._)
#.add x y
#.map f 
(* translated *)
(M.map (M.add data  x y))
(data : List._)
#.foldRight ~init:[] ~f:List.cons
#.map (fun x -> x + 1)
@OvermindDL1
Copy link
Contributor

Would this work when the code is compiled with the stock OCaml compiler when compiling to native code (kind of need that for native server-side template generation).

/me is really not liking things that are not fully and completely backwards compatible, and not just via a system check

@bobzhang
Copy link
Member Author

bobzhang commented Mar 7, 2018

other potential operators (|#), (|.)

empty 
|# M.add 1 2 
|# M.add 1 2 
|# M.reduce 1 2 (fun x -> ...)

@bobzhang bobzhang changed the title real pipe using #. real pipe using |# Mar 7, 2018
@chenglou
Copy link
Member

chenglou commented Mar 9, 2018

Let's go with |., since this was originally proposed for Reason and people seemed receptive.

@yawaramin
Copy link
Contributor

A couple of things we should check or coordinate:

  • Is |. currently in usage in any OCaml codebase that we can reasonably get our hands on. E.g., I know the OCaml folks at Cambridge (I think Anil Madhavapeddy's team) have a build farm that they use to check for potentially breaking changes on package version upgrades. Could we get access to that to just run a grep for |.?
  • Can we coordinate support for |. with our officially recommended editor tooling? That would be amazing to pull off :-D

@bobzhang
Copy link
Member Author

bobzhang commented Mar 9, 2018

@chenglou the main thing I am worried using (|.) may already be used by existing libs, while (|#) are relatively safe. We can have an option to turn it on/off, but it would be nice to make it unneeded in most cases

@bobzhang
Copy link
Member Author

bobzhang commented Mar 12, 2018

It seems |# is not a valid operator
according to http://caml.inria.fr/pub/docs/manual-ocaml/lex.html#operator-char

Such options are available:

|!
|$
|%
|&
|*
|+
|-
|.
|/
|:
|<
|=
|>
|?
|@
|^
||
|~

what do you think? Tend to reserve |.

@yawaramin
Copy link
Contributor

First choice |.
Second choice |/

@chenglou
Copy link
Member

|. is what Reason would use if we ever need pipe-first (most likely don't anymore since |> _. So that's my preference as well.

@chenglou
Copy link
Member

Let's check for conflict then go with |. then. Not sure how to coordinate on the editor side. @yawaramin feel free to add this token to the list of infix stuff or something.

@chenglou chenglou changed the title real pipe using |# Syntax for piping into the first argument Mar 13, 2018
@chenglou
Copy link
Member

Someone should go and clone every ocaml package and grep for |. lol

bobzhang added a commit that referenced this issue Mar 13, 2018
bobzhang added a commit that referenced this issue Mar 13, 2018
@Risto-Stevcev
Copy link

Isn't it better not to use a valid operator here? I thought that's why |# was suggested first
This seems like an extremely bad idea

@bobzhang
Copy link
Member Author

We will prohibit people using custom defined |. operator, so soundness will be preserved.
There is no plan to change the syntax, the cost would be too high

@Risto-Stevcev
Copy link

Risto-Stevcev commented Mar 13, 2018

I was thinking in terms of porting ocaml libraries that already have it defined and using it internally in that library

@yawaramin
Copy link
Contributor

yawaramin commented Mar 13, 2018

Yeah. Like @chenglou said we need to figure out if there would be a big impact, i.e. are people actually defining |. as an infix operator. OK. Rather than waiting for a perfect solution, I'll send out a couple of posts on the active OCaml channels that I know of and see what people say.

@bobzhang
Copy link
Member Author

I dont think the operator is used in the wild, but I will send an email to caml-list in next release
This is a very small change, and the work around is trivial, I think the benefit justifies the cost

@Risto-Stevcev
Copy link

What about adding a flag in bsconfig.json to be able to disable it on a per-project basis?
That way if there's a compat issue with some ocaml library it won't affect anything. We'll be able to use a library that has it turned off as a dependency and yet still use the new syntax in the project.

It's a really big change and there's no going back, that's why I'm really hesitant about this. If a really influential ocaml library comes around in the future and happens to use |. somewhere, then a bucklescript port would have to maintain a fork of the repo. The workaround would only be trivial in trivial cases too. It gets even more hairy if it depends on something that does that, or depends on something that depends on something that does that, etc.

@utkarshkukreti
Copy link
Contributor

I just downloaded all 1805 packages from OPAM and ran rg -F '(|.)' on it. I found 8 packages that define that operator:

d3.0.3.0
gapi-ocaml.0.3.6
glsurf.3.3.1
hardcaml.1.2.0
hardcaml-bloop.0.1.0
lens.1.2.0
operf-micro.1.1.1
uint.1.2.1

@chenglou
Copy link
Member

@utkarshkukreti thanks a lot! This community is crazy great
@Risto-Stevcev these are native packages, and it's just the regular bs ppx, so we can disable it in the future easily.
But as we've seen above, there's not that much usage. Only two packages or so are downloaded more than 10 times a month (and I suspect lots as transitive deps). |. it is I think.

@bobzhang
Copy link
Member Author

There is a paradox here, if we allow such work around, then people would use it. We can always make it configurable in the future if something can not be fixed. But I think it is better to make it strict here, so people would adopt such convention

bobzhang added a commit that referenced this issue Mar 14, 2018
@Risto-Stevcev
Copy link

One other thing I was wondering about -- how's the interop with the existing pipe operator?
For example, if there are some target-first functions and some target-last functions that need threading, is it possible to do:

target
|. targetFirstFn
|. targetFirstFn2(foo)
|> targetLastFn(bar)
|. targetFirstFn3

@bobzhang
Copy link
Member Author

it should just work as it is. We did not change the semantics of existing pipe operator

@chenglou
Copy link
Member

chenglou commented Jul 2, 2018

Stale

@chenglou chenglou closed this as completed Jul 2, 2018
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

6 participants