Missing => infix operator escape#1944
Conversation
| let escape_stars_slashes str = | ||
| if String.contains str '/' then | ||
| if (String.contains str '/') || | ||
| ((String.contains str '=') && (String.contains str '>')) then |
There was a problem hiding this comment.
maybe this can be a simple str = "=>" since we're special casing the fat arrow. let me know
There was a problem hiding this comment.
This probably should be str = "=>" or something equivalent to avoid catching operators which don't need escaping like >= or ==>.
There was a problem hiding this comment.
@hcarty that a better reason to do what I was suggesting, will fix.
|
Great improvement, @anmonteiro! One alternative approach would be to swap |
|
One downside to swapping is that it breaks with how other conflicts are handled between syntaxes. |
|
@hcarty what do you mean? edit: sorry I hadn't seen @jordwalke's comment. |
|
@jordwalke I personally don't mind it, here are my thoughts on the tradeoffs:
Thoughts? |
|
It might be nice to save The previously cases of swapping for keywords caused confusion before. Infix operators are not likely to be as confusing when swapped - that's already done for string concatenation for example - but it's still nice to have conversion rules be consistent. Is |
|
@jordwalke do you agree with the above? should we go ahead and keep |
79fef91 to
f9fc39a
Compare
|
My original idea of swapping |
|
The reasons for using such a complicated escaping convention for One approach: The downside to that approach is that operators are extra long etc (even ones like Another approach is that we can swap the operator
|
|
@jordwalke I really like your 2nd idea. I'll try to prototype something in this PR over the next couple of days. I think you said in Discord that backslash prefixed operators are not valid in OCaml, which would allow Reason to fix all these compatibility parsings in one go. |
|
A user pointed out that |
|
@hcarty the idea I have in mind (and I think it's what Jordan implied) is that That solves the problem, right? |
|
@anmonteiro I think so, at least for infix operators. |
This PR applies the same solution that we use for `/\*` and makes `=\>` work fixes reasonml#1941
|
@jordwalke I think this is ready for another look. I implemented your suggestion by having any operator preceded with a backslash ( This enables things like: let (\++) = ...
let (\=>) = ...The way I made it generic for all backslash-prefixed operators is by still having e.g. The downside of this approach is that converting OCaml code that has e.g. |
|
This seems like a nice simplification, with the downside that keeping the |
|
@hcarty The problem is only about OCaml -> Reason, not the other way around. Some examples: # Reason -> Reason
$ echo 'let (\=>) = (a, b) => a + b' | ./_build/install/default/bin/refmt
let (\=>) = (a, b) => a + b;
$ echo 'let (\++) = (a, b) => a + b' | ./_build/install/default/bin/refmt
let (\++) = (a, b) => a + b;
# Reason -> OCaml
$ echo 'let (\=>) = (a, b) => a + b' | ./_build/install/default/bin/refmt --print ml
let (=>) a b = a + b
$ echo 'let (\++) = (a, b) => a + b' | ./_build/install/default/bin/refmt --print ml
let (++) a b = a + b
# OCaml -> Reason (where the problem is)
$ echo 'let (=>) a b = a + b' | ./_build/install/default/bin/refmt --parse ml
let (=>) = (a, b) => a + b;
$ echo 'let (++) a b = a + b' | ./_build/install/default/bin/refmt --parse ml
let (++) = (a, b) => a + b; |
|
@anmonteiro Sorry, I was thinking of Reason -> OCaml through ocamlformat since refmt doesn't keep comments. I don't think ocamlformat is perfect in that regard either, to be fair. And ocamlformat's Reason -> OCaml conversion currently has some pretty tight version constraints. |
|
@hcarty If you refmt Reason -> Ocaml with refmt and then use ocamlformat, would that work in your case? |
|
@IwanKaramazow Unfortunately not. Going |
|
Thanks for the work here @anmonteiro! It'd be nice to get this PR approved and merged, since it blocks interoperability with some fairly basic OCaml infix operators, esp. |
|
The PR isn't done yet, and I haven't found the time to squeeze it in. |
|
A nasty one I just come across is the |
This PR applies the same solution that we use for
/\*and makes=\>work.Because we reuse the solution that is already in place, this is still parsed as
=>in the AST and printed as=>in the OCaml side.fixes #1941