Display error location inline #1857
Conversation
jscomp/ext/ext_color.mli Outdated
| | ||
| (** Input is the tag for example `@{<warning>@}` return escape code *) | ||
| val ansi_of_tag : string -> string | ||
| val ansi_of_tag : ?style_of_tag:(string -> style list) -> string -> string |
There was a problem hiding this comment.
looks like the ability to inject a style rather than relying or extending https://github.com/chenglou/bucklescript/blob/944d705c6d7faff3fceffae0aef4728036817b52/jscomp/ext/ext_color.ml#L80?
There was a problem hiding this comment.
Yeah see Super_misc.colorize_tagged_string
There was a problem hiding this comment.
do we really need that flexibility?
| open Printtyp | ||
| | ||
| open Location | ||
| |
There was a problem hiding this comment.
can you add a .mli as well, in general, the less open, the better
| | ||
| done; | ||
| fprintf ppf "@]" (* v *) | ||
| |
| | ||
| if current_line_strictly_between_start_and_end_line then fprintf ppf "@}"; | ||
| | ||
| fprintf ppf "@]"; (* hov *) |
There was a problem hiding this comment.
are you aware of %a, in general, people use pp_open_box explicitly or [@%a@]
There was a problem hiding this comment.
Did do that. Then lost track of boxes in the same function. I can refactor that later if needed
| Anyone interested in reviewing this PR? |
jscomp/ext/ext_color.mli Outdated
| | ||
| (** Input is the tag for example `@{<warning>@}` return escape code *) | ||
| val ansi_of_tag : string -> string | ||
| val ansi_of_tag : ?style_of_tag:(string -> style list) -> string -> string |
There was a problem hiding this comment.
Yeah see Super_misc.colorize_tagged_string
| | ||
| if current_line_strictly_between_start_and_end_line then fprintf ppf "@}"; | ||
| | ||
| fprintf ppf "@]"; (* hov *) |
There was a problem hiding this comment.
Did do that. Then lost track of boxes in the same function. I can refactor that later if needed
| Done! |
jscomp/super_errors/super_misc.mli Outdated
| val print_file: range:(int * int) * (int * int) -> lines:string array -> Format.formatter -> unit -> unit | ||
| | ||
| (** This will be called in super_main. This is how you override the default error and warning printers *) | ||
| val colorize_tagged_string: Format.formatter -> (string -> Ext_color.style list) -> unit |
There was a problem hiding this comment.
why do we need different tags? I would like to keep its simplicity as it is now
There was a problem hiding this comment.
I do already use different tags, one set for warnings, one set for errors and one for that reporter; each reporter would probably get a different set too, and if not, they can be shared among them.
There was a problem hiding this comment.
can we keep it simple? we can add more tags if info, warn, error is not enough. But I don't think the benefit of having such flexibility justify its complexity. After all, people can only tell 3 or 4 colors
There was a problem hiding this comment.
I'm not using it for warn/error etc. I'm using it like so: https://github.com/BuckleScript/bucklescript/pull/1857/files#diff-98efb70d9d6c67e672eaf4a1a529e112R44
This way these printing functions don't need to know whether they're emitting a warning, an error, or a themed color in the future (some colors don't work well at all on the white terminal theme or windows blue).
If you want a single one then I can gather these (including maybe a few dozens of https://github.com/BuckleScript/bucklescript/pull/1857/files#diff-4b8762e0b86bfe6187fbc1eca3f9d139R237) under a single switch?
There was a problem hiding this comment.
I've tried just adding tags to ext_colors but then I realized it's not actually called. The existing colors are called by bsb_main's Bsb_log.setup (), and maybe at that point it's already too late, the ninja command has exited (correct me if I'm wrong).
So I'll end up with a Super_misc.setup, or the current colorize_tagged_string anyway, and call it at the appropriate time. But if you'd like to avoid the complexity of adding the option arg in ext_colors, I can copy it over to a super_* file?
There was a problem hiding this comment.
Note that both bsb and bsc produce colorful error message but with different setup. (Bsb_log.setup and https://github.com/bucklescript/bucklescript/blob/master/vendor/ocaml/utils/misc.ml#L463 for the compiler)
I have a feeling that semantic tags is the wrong abstraction,(it is neither type safe nor scalable) can we hold on not over-using it for a while(just use existing provided tag defined in the compiler)?
There was a problem hiding this comment.
Check
https://github.com/bucklescript/bucklescript/blob/master/vendor/ocaml/utils/misc.ml#L422
There is already a predefined tag loc
| Ok, I've killed the custom semantic tags (only added two needed ones to ext_color now, no extra anywhere else). |
| thanks! |
Gonna be testing the recent commits internally!
3 participants
This makes it so that turning on
-bs-better-errorshows errors inline, a big request from the community.It gracefully degrades to the normal reporting.
