[Super errors] Specialize error msgs for Reason syntax when needed #2616
Conversation
|
Before: Same messages tweaks for the OCaml syntax. That floating dot is the actual |
jscomp/core/js_main.ml
Outdated
| Arg.Unit Reason_outcome_printer_main.setup, | ||
| Arg.Unit (fun () -> | ||
| Reason_outcome_printer_main.setup (); | ||
| Super_reason_flag.using_reason_syntax := true |
There was a problem hiding this comment.
@bobzhang is this the right way to do it?
| _ | ||
| ) | ||
| -> | ||
| if !Super_reason_flag.using_reason_syntax then |
There was a problem hiding this comment.
I suppose this module only print reason syntax, lets keep things simple, make improved error message only work for reason syntax ?
Currently, we did not activate better error message for ocaml syntax. In that case we don't need add a new state variable here
There was a problem hiding this comment.
The reason syntax printer is in a different module. This module’s logic is shared with typecore. Also, I’m pretty positive lots of pure ocaml projects enable super errors manually. Bsb is also in vscode and atom plugins’ configurations now, so we can’t skim on these features imo
There was a problem hiding this comment.
Can we do it in a separate PR so that we can merge this strictly better PR quickly -- I think we already had something relevant to whether reason syntax is enabled, but not sure where
There was a problem hiding this comment.
How do I split the PR? Also, the only thing I've seen that knows whether it's a reason file is Reason_outcome_printer_main.setup(). I can't put the flag there because of cyclic dependencies.
There was a problem hiding this comment.
I mean just always print it in reason syntax (not introducing a ref variable), does it work?
There was a problem hiding this comment.
We can't, because pure ocaml bs projects are using super-errors right now
There was a problem hiding this comment.
The reason setup logic is already pretty complex (to be honest, I don't 100% understand how they interact).
My impression is that super-errors is currently only activated for reason syntax, let's not over-promise, having a cursory look at super_typecore module, print_arguments is reason specific, I am not really sure this module is not reason syntax specific.
I think we should not make the setup logic more and more complex to deliver things we did not promise
There was a problem hiding this comment.
We're not over-promising, we're just not breaking existing experience. That print_arguments has one reason syntax-specific line because I missed it, but it's easy to make it work with ocaml syntax too.
Take a look at this result: https://github.com/search?q=-bs-super-errors&type=Code&utf8=✓
Some prominent projects, including the graphql integration (pure ocaml), uses super-errors.
There was a problem hiding this comment.
I don't think we're over-promising here. We've never really had reason-specific logic in super-errors (printing the outcome tree in reason is independent of super-errors. It's used elsewhere too), except that print_arguments which is my fault
See the updated error/warning msgs As per feedback, we're gonna do it for Reason syntax for now. We'll add back the support for OCaml syntax later.
|
As per feedback, we're gonna do it for Reason syntax for now. We'll add |
|
Alright, since this is now just two messages tweaks, I'll merge it. I'll submit the reason flag as a different PR |
…g reason or ocaml syntax See rescript-lang#2616 (comment)
…g reason or ocaml syntax See rescript-lang#2616 (comment)
See the updated error/warning msgs