Switch from ANSITerminal to ocolor #474

AltGr · 2023-06-08T10:23:03Z

ocolor uses OCaml's Format module support for semantic tags; hence we can keep
using the usual formatting functions, but use format-string tags such as
@{<blue>some blue text@} inside them. There are several benefits, including
readability, correct formatting (the format characters are automatically skipped
from column calculations), and correct nesting of tags.

To properly leverage this, this PR also cleans up the whole printing pipeline,
ensuring we use Formatters all the way through (we used to have intermediate
string conversions in many places, which breaks the formatting paradigm).

In addition, this allows us to delay the decision to print color codes or not to
the backend printer, e.g. printing functions are not concerned wether color is
enabled, and it is done by detecting if the end output is a tty or a file (this
can still be forced by the --color option of course)

Another small change is that errors, warnings and logs are now all printed to
stderr, reserving stdout to normal program output (e.g. a .dcalc file that you
may want to redirect)

(and some cleanup of the remaining deps to ANSITerminal)

AltGr · 2023-06-08T10:37:02Z

Closes #451

denismerigoux

Thanks! This is so cool!

compiler/catala_utils/cli.mli

compiler/catala_utils/messages.ml

denismerigoux · 2023-06-08T11:46:45Z

compiler/catala_utils/messages.ml

-  if !Cli.disable_warnings_flag then Format.ifprintf Format.std_formatter format
-  else Format.printf ("%a" ^^ format ^^ "\n%!") warning_marker ()
+let get_ppf = function
+  | Result -> Lazy.force std_ppf


Why Lazy ?

Tere are a few reasons:

we want to do the setup only once (isatty is not free ; and the Format setup uses side-effects)

we need the command-line to already have been parsed before we can do that setup¹

also, we recreate a formatter at each call (for consistency among formatters for file output, etc.) and certainly don't want to use multiple concurrent formatters to stdout (that could be fixed by using Ocolor_format.raw_std_formatter instead though)

¹ I agree that this is a bit fragile if one were to use Messages printing too early in the process; we could switch to hidden references and explicitely call setup functions upon parsing the command-line if that were a problem (but that would also be an issue with cli flags not consistently acknowledged).

There should be this comment in messages.mli that describes how we expect these side-effects to work I think :)

denismerigoux · 2023-06-08T11:48:12Z

compiler/catala_utils/messages.ml


-  let of_message (s : string) : t = { message = s; positions = [] }
+  let of_string (s : string) : t =
+    { message = (fun ppf -> Format.pp_print_string ppf s); positions = [] }


I think the codebase of the compiler uses fmt instead of ppf as the go-to variable names for formatters.

Hm, I know (or at least it used to 😇)
The reason for my preference is that you often have functions that take a pretty-printing formatter and a format string as arguments, and fmt is ambiguous in that regard

denismerigoux · 2023-06-08T11:48:50Z

compiler/catala_utils/messages.ml

-                (match pos.message with None -> "" | Some msg -> msg ^ "\n")
-                (Pos.retrieve_loc_text pos.position))
-            pos))
+    format target "@[<v>%t%a@]" message


Wouahou what's %t ?

Sorry if it's a bit obscure, it's a function that just takes a ppf as argument. In the above "%t" message is basically equivalent to "%a" (fun ppf () -> message ppf) ()

Thanks, didn't know!

denismerigoux · 2023-06-08T11:50:34Z

compiler/desugared/linting.ml

-              (Format.asprintf "\"%a\"" ScopeName.format_t scope_name)
-              (Cli.format_with_style [ANSITerminal.yellow])
-              (Format.asprintf "\"%a\"" Ast.ScopeDef.format_t scope_def_key))
+              "In scope @{<yellow>\"%a\"@}, the variable @{<yellow>\"%a\"@} is \


This is a big win!!!!

AltGr · 2023-06-08T13:32:39Z

Thanks @denismerigoux for the review; I added to the PR the modified output of tests so you may want to double-check if there is no issue with that (tests were passing but that's because they tolerate whitespace diffs)

denismerigoux · 2023-06-08T13:51:03Z

tests/test_array/good/fold.catala_en

@@ -32,7 +32,8 @@ scope B:
 ```catala-test-inline
 $ catala Interpret -s A
 [RESULT] Computation successful! Results:
-[RESULT] x =
+[RESULT]
+x =


Not sure that we want this newline here between [RESULT] and x

Thing is, since we now ensure that pretty-printing is done consistently, the code below will be aligned with the x = ; so there is no clean/easy way to have neither the newline, nor the extra indentation as in the ERROR examples below.

Hum hum, the downside of doing things correctly :) Then we can leave it as it is in this PR right now.

denismerigoux · 2023-06-08T13:51:40Z

examples/NSW_community_gaming/tests/test_nsw_social_housie.catala_en

@@ -29,8 +29,8 @@ scope Test1:
 ```catala-test-inline
 $ catala Interpret -s Test1
 [ERROR] Syntax error at token "scope"
-Message: expected either 'condition', or 'content' followed by the expected variable type
-Autosuggestion: did you mean "content", or maybe "condition"?
+        Message: expected either 'condition', or 'content' followed by the expected variable type


Do we want that extra indent ? Also why are we keeping under CI something that has a syntax error?

denismerigoux · 2023-06-08T13:52:11Z

tests/test_bool/bad/bad_assert.catala_en

@@ -13,8 +13,8 @@ scope Foo:
 ```catala-test-inline
 $ catala Interpret -s Foo
 [ERROR] Error during typechecking, incompatible types:
--> integer
--> bool
+        --> integer


Not sure we want this big indent

In this case (comparing with the above RESULT), we can probably tweak the printer for the specific error message to not have an outer box that creates this alignment.

denismerigoux · 2023-06-08T13:53:32Z

tests/test_exception/good/groups_of_exceptions.catala_en

-[RESULT] Printing the tree of exceptions for the definitions of variable "x" of scope "Foo".
-[RESULT] Definitions with label "base":
+[RESULT]
+Printing the tree of exceptions for the definitions of variable "x" of scope "Foo".


Yeah I guess the newline here is a bit ugly

AltGr added 3 commits June 8, 2023 12:11

Use ocolor instead of ANSITerminal

deaf407

Reformat

16c93fb

Correctly setup ocolor on all output channels

c564e70

(and some cleanup of the remaining deps to ANSITerminal)

Repair indentation of log messages

11730ce

denismerigoux approved these changes Jun 8, 2023

View reviewed changes

AltGr added 2 commits June 8, 2023 14:03

Update tests with whitespace changes

fcadd98

Tweak whitespace in messages

e4c1216

denismerigoux reviewed Jun 8, 2023

View reviewed changes

AltGr merged commit 9577d57 into CatalaLang:master Jun 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from ANSITerminal to ocolor #474

Switch from ANSITerminal to ocolor #474

AltGr commented Jun 8, 2023

AltGr commented Jun 8, 2023

denismerigoux left a comment

denismerigoux Jun 8, 2023

AltGr Jun 8, 2023

denismerigoux Jun 8, 2023

denismerigoux Jun 8, 2023

AltGr Jun 8, 2023

denismerigoux Jun 8, 2023

denismerigoux Jun 8, 2023

AltGr Jun 8, 2023

denismerigoux Jun 8, 2023

denismerigoux Jun 8, 2023

AltGr commented Jun 8, 2023

denismerigoux Jun 8, 2023

AltGr Jun 8, 2023

denismerigoux Jun 8, 2023

denismerigoux Jun 8, 2023

denismerigoux Jun 8, 2023

AltGr Jun 8, 2023

denismerigoux Jun 8, 2023

Switch from ANSITerminal to ocolor #474

Switch from ANSITerminal to ocolor #474

Conversation

AltGr commented Jun 8, 2023

AltGr commented Jun 8, 2023

denismerigoux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AltGr commented Jun 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment