feat(Tty): custom markers

[favonia]

The current code works but I'm not sure about the API. With this PR, the Tty.S.display function takes a new argument marker of type

use_ansi:bool -> use_color:bool -> [`End_of_line | `End_of_file] option -> tag mark -> string

that turns a "mark" into a string for display. I wonder how obvious this design is to library users without reading any documentation?

[mikeshulman]

I wonder how obvious this design is to library users without reading any documentation?

My initial reaction is "not very". (-:

I'm guessing that the output of this function is supposed to be a string like "EOF" or "EOL". If so, why does it need to know about use_ansi and use_color? And what is a tag MarkedSource.mark?

[favonia]

@mikeshulman Your guess is correct. The reason for use_ansi is that a "mark" can also signify the start or end of a range. Without ANSI control sequences, marking ranges would require additional symbols. Currently, we don't use any extra symbols, even when ANSI control sequences are disabled. (Related: #131)

Here's the definition of mark:

(** A mark signals the start or end of a non-empty range, or the location of a point (a range of zero width). *)
type 'tag mark = 'tag MarkedSource.mark =
  | RangeBegin of 'tag
  | RangeEnd of 'tag
  | Point of 'tag

[mikeshulman]

What is the tag?

Are all possible combinations of values for the [`End_of_line | `End_of_file] option and the tag mark arguments possible? E.g. could you have a Some `End_of_file combined with RangeBegin?

[favonia]

@mikeshulman Here's the definition of tag:

(** The index type of all messages in a diagnostic. [ExtraRemark i] is the [i]th extra remark (indexes start from zero). *)
type index = MainMessage | ExtraRemark of int

(** A tag consists of an index (of type {!type:index}) and a message (of type {!type:Text.t}) *)
type tag = index * Text.t

It should be impossible to have Some `End_of_file with RangeBegin.

From the conversation, I guess my dream of "making the API self-explanatory" failed... but I'm not sure how to improve it.

[mikeshulman]

My rule of thumb is that if some combination of arguments is impossible, then the types should prevent that combination from occurring. Can those two arguments be combined into one, belonging to a datatype that enumerates the exact possibilities, exhaustively and non-overlappingly, with each constructor taking a fully flattened tuple of arguments that doesn't require looking up the definitions of other types to know what it contains?

[favonia]

@mikeshulman Good point! I'll need more time to polish it.

Another impossible combination is ~use_ansi:false and ~use_color:true. I guess these two can be combined into three options:

• Disabled: No ANSI control sequences.
• No_color: Use ANSI control sequences, but without colors.
• Fully_enabled: Use colors.

PS: for the display function, the same combination (~use_ansi:false and ~use_color:true) is also impossible, but there are 9 combinations there (enable/disable/auto)^2 instead of just 4 (enabled/disabled)^2. I gave up on naming all 8 options...

[mikeshulman]

In the display function, in practice would anyone really want any combination other than the four of Disabled, No_color, Fully_enabled, and Auto?

[favonia]

@mikeshulman Here is the table. (There are actually only 7 valid choices because two have the same effect:)

	use_ansi:true	use_ansi:false	?use_ansi unset
~use_color:true	Enabled_with_color (Fully_enabled)	(error)	Disabled_or_with_color
~use_color:false	Enabled_without_color (No_color)	Disabled	Disabled_or_without_color
?use_color unset	Enabled_with_or_without_color	Disabled	Auto

I can imagine that colorblind people would want Disabled_or_without_color? Enabled_with_or_without_color can force producing ANSI sequences (but possibly without colors) for file redirection. However, I'm not sure how Disabled_or_with_color is useful---maybe to cancel the effect of NO_COLOR=1?

[mikeshulman]

Ok, I guess 7 is close enough to 3x3 that having two arguments does make sense there. (-:

[favonia]

Current progress

I hope this type looks better now...

type marker =
  ansi:[ `Enabled_with_color | `Enabled_without_color | `Disabled ]
  -> [ `Main_message | `Extra_remark of int ]
  -> [ `Range_begin
     | `Range_end of [`End_of_line | `End_of_file] option
     | `Point of [`End_of_line | `End_of_file] option
     ]
  -> string

Remaining issues

1. What should be the default range delimiters when ANSI sequences is disabled?
2. Should we allow custom symbols before texts? Current style, without any symbols:

   ￭ /path/to/file
   23 | wwww‹POS›wwwwww‹EOF›
      ^ somewhere in the middle
      ^ ending of another file

   Alternative style 1:

   ￭ /path/to/file
   23 | wwww‹POS›wwwwww‹EOF›
      ^ ‹POS›
        somewhere in the middle
      ^ ‹EOF›
        ending of another file
        second line of message

   Alternative style 2:

   ￭ /path/to/file
   23 | wwww‹POS›wwwwww‹EOF›
      ^ ‹POS› somewhere in the middle
      ^ ‹EOF› ending of another file
        second line of message

[favonia]

Let me merge this first so that I can work on other radical changes. :sweat_smile:

[favonia]

Update: now French double quotation marks (angle quotation marks) are used for non-empty ranges (e.g., «let x = 1») when ANSI escape sequences are disabled. This is probably the most compatible choice with the single quotes for point marks (e.g., ‹EOF›). (Maybe we should swap the single/double quotation marks?)

[mikeshulman]

The French quotes look fine to me. I would probably lean towards "alternative style 2".

The type of marker looks much better now, thanks. Minor thoughts:

• Why is the ansi argument labeled but not the others?
• I think it would be clear enough for the ansi argument to be [ `Color | `No_color | `Disabled ]; the presence of a Disabled option implies that the others are versions of enabled.
• What does the int argument of Extra_remark mean?
• I would be inclined to write [`End_of_line | `End_of_file] option instead as something like [`End_of_line | `End_of_file | `Other ], but I can see an argument the other way too.

[favonia]

@mikeshulman Thanks. For Question 1, I was just not sure what constructor names to use without the labeling. What constructors would you suggest if the ansi argument is not labeled? Would you suggest [ `Color | `No_color | `No_ansi ]? Extra_remark i means the ith extra remark in the original input.

[mikeshulman]

Hmm, maybe. No_color is a bit misleading on its own since it doesn't indicate what should be used.