Skip to content

Commit

Permalink
fix: add break hints to Dyn.pp (ocaml#8980)
Browse files Browse the repository at this point in the history 
When working on another feature I noticed that Dyn.pp behaved very
poorly when pretty printing variant contructor arguments. Turns out a
Pp.char ',' seperator was being used however there was no break hint.
This patch adds a Pp.space afterwards which seems to make the output
much more pleasant.

Signed-off-by: Ali Caglayan <alizter@gmail.com>
  • Loading branch information
@Alizter
Alizter authored Oct 20, 2023
1 parent afa34ed commit f6c4ad3
Show file tree
Hide file tree
Showing 5 changed files with 240 additions and 300 deletions.
6 changes: 5 additions & 1 deletion otherlibs/dyn/dyn.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,11 @@ let rec pp =
| Variant (v, xs) ->
Pp.hvbox
~indent:2
(Pp.concat [ Pp.verbatim v; Pp.space; Pp.concat_map ~sep:(Pp.char ',') xs ~f:pp ])
(Pp.concat
[ Pp.verbatim v
; Pp.space
; Pp.concat_map ~sep:(Pp.seq (Pp.char ',') Pp.space) xs ~f:pp
])
;;

let to_string t = Format.asprintf "%a" Pp.to_fmt (pp t)
Expand Down
12 changes: 6 additions & 6 deletions otherlibs/ocamlc-loc/test/ocamlc_loc_tests.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ Error: Signature mismatch:
[%expect
{|
>> error 0
{ loc = { path = "test.ml"; line = Range 3,5; chars = Some (6, 3) }
{ loc = { path = "test.ml"; line = Range 3, 5; chars = Some (6, 3) }
; message =
"Signature mismatch:\n\
Modules do not match:\n\
Expand Down Expand Up @@ -316,7 +316,7 @@ Error: Some record fields are undefined: signal_watcher
>> error 0
{ loc =
{ path = "test/expect-tests/timer_tests.ml"
; line = Range 6,10
; line = Range 6, 10
; chars = Some (2, 3)
}
; message = "Some record fields are undefined: signal_watcher"
Expand Down Expand Up @@ -573,7 +573,7 @@ Case
>> error 0
{ loc =
{ path = "src/dune_engine/action.ml"
; line = Range 34,96
; line = Range 34, 96
; chars = Some (4, 64)
}
; message =
Expand All @@ -586,7 +586,7 @@ Case
>> error 1
{ loc =
{ path = "src/dune_engine/action.ml"
; line = Range 291,315
; line = Range 291, 315
; chars = Some (2, 22)
}
; message =
Expand All @@ -599,7 +599,7 @@ Case
>> error 2
{ loc =
{ path = "src/dune_engine/action.ml"
; line = Range 339,363
; line = Range 339, 363
; chars = Some (21, 24)
}
; message =
Expand All @@ -612,7 +612,7 @@ Case
>> error 3
{ loc =
{ path = "src/dune_engine/action.ml"
; line = Range 391,414
; line = Range 391, 414
; chars = Some (4, 70)
}
; message =
Expand Down
229 changes: 94 additions & 135 deletions otherlibs/stdune/test/ansi_color_tests.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@ let%expect_test "reproduce #2664" =
[%expect
{|
Vbox
0,Seq
0,
Seq
Seq
Seq
Seq
Seq
Expand All @@ -72,46 +74,27 @@ let%expect_test "reproduce #2664" =
Seq
Seq
Seq
Seq
Nop,Tag [ Fg_blue ],Verbatim "1",
Tag
[ Fg_blue ],Verbatim "2",
Tag
[ Fg_blue ],Verbatim "3",Tag
[ Fg_blue
],
Verbatim
"4",
Tag
[ Fg_blue ],Verbatim "5",Tag
[ Fg_blue ],
Verbatim
"6",
Tag
[ Fg_blue ],Verbatim "7",Tag
[ Fg_blue ],
Verbatim
"8",Tag
[ Fg_blue
],
Verbatim
"9",
Tag
[ Fg_blue ],Verbatim "10",Tag
[ Fg_blue ],Verbatim
"11",
Tag
[ Fg_blue ],Verbatim "12",Tag
[ Fg_blue ],Verbatim "13",
Tag
[ Fg_blue ],Verbatim "14",Tag [ Fg_blue ],Verbatim "15",
Tag
[ Fg_blue ],Verbatim "16",Tag [ Fg_blue ],Verbatim "17",
Tag
[ Fg_blue ],Verbatim "18",Tag [ Fg_blue ],Verbatim "19",Tag
[ Fg_blue
],Verbatim
"20" |}]
Nop,
Tag [ Fg_blue ], Verbatim "1",
Tag [ Fg_blue ], Verbatim "2",
Tag [ Fg_blue ], Verbatim "3",
Tag [ Fg_blue ], Verbatim "4",
Tag [ Fg_blue ], Verbatim "5",
Tag [ Fg_blue ], Verbatim "6",
Tag [ Fg_blue ], Verbatim "7",
Tag [ Fg_blue ], Verbatim "8",
Tag [ Fg_blue ], Verbatim "9",
Tag [ Fg_blue ], Verbatim "10",
Tag [ Fg_blue ], Verbatim "11",
Tag [ Fg_blue ], Verbatim "12",
Tag [ Fg_blue ], Verbatim "13",
Tag [ Fg_blue ], Verbatim "14",
Tag [ Fg_blue ], Verbatim "15",
Tag [ Fg_blue ], Verbatim "16",
Tag [ Fg_blue ], Verbatim "17",
Tag [ Fg_blue ], Verbatim "18",
Tag [ Fg_blue ], Verbatim "19",
Tag [ Fg_blue ], Verbatim "20" |}]
;;

let%expect_test "Ansi_color.strip" =
Expand Down Expand Up @@ -146,7 +129,9 @@ let%expect_test "parse fg and bg colors" =
[%expect
{|
Vbox
0,Seq
0,
Seq
Seq
Seq
Seq
Seq
Expand All @@ -156,27 +141,18 @@ Vbox
Seq
Seq
Seq
Seq
Seq Nop,Verbatim "This is a ",Tag
[ Fg_blue ],
Verbatim
"blue",Verbatim
" string with ",
Tag
[ Fg_red ],Verbatim "red",Verbatim " and ",Tag
[ Fg_green
],
Verbatim
"green",
Verbatim
" together with strings of a ",Tag
[ Bg_blue ],Verbatim
"blue blackground",
Verbatim
" and ",Tag [ Bg_red ],Verbatim "red background",Verbatim
" and ",
Tag
[ Bg_green ],Verbatim "green background" |}]
Seq Nop, Verbatim "This is a ",
Tag [ Fg_blue ], Verbatim "blue",
Verbatim " string with ",
Tag [ Fg_red ], Verbatim "red",
Verbatim " and ",
Tag [ Fg_green ], Verbatim "green",
Verbatim " together with strings of a ",
Tag [ Bg_blue ], Verbatim "blue blackground",
Verbatim " and ",
Tag [ Bg_red ], Verbatim "red background",
Verbatim " and ",
Tag [ Bg_green ], Verbatim "green background" |}]
;;

let%expect_test "parse multiple fg and bg colors" =
Expand All @@ -191,16 +167,14 @@ let%expect_test "parse multiple fg and bg colors" =
[%expect
{|
Vbox
0,Seq
0,
Seq
Seq
Seq
Seq
Seq Nop,Verbatim "This text is ",Tag
[ Fg_blue; Bg_red ],Verbatim
"blue string with a red background",
Verbatim
" and ",Tag
[ Fg_green; Bg_blue ],Verbatim
"green string with a blue background" |}]
Seq Nop, Verbatim "This text is ",
Tag [ Fg_blue; Bg_red ], Verbatim "blue string with a red background",
Verbatim " and ",
Tag [ Fg_green; Bg_blue ], Verbatim "green string with a blue background" |}]
;;

let%expect_test "fg default overrides" =
Expand All @@ -215,15 +189,14 @@ let%expect_test "fg default overrides" =
[%expect
{|
Vbox
0,Seq
0,
Seq
Seq
Seq
Seq
Seq Nop,Verbatim "This text has a ",Tag
[ Fg_blue ],Verbatim
"blue foreground",
Verbatim
" but here it becomes the default foreground,",Verbatim
" even together with another foreground modifier." |}]
Seq Nop, Verbatim "This text has a ",
Tag [ Fg_blue ], Verbatim "blue foreground",
Verbatim " but here it becomes the default foreground,",
Verbatim " even together with another foreground modifier." |}]
;;

let%expect_test "bg default overrides" =
Expand All @@ -238,15 +211,14 @@ let%expect_test "bg default overrides" =
[%expect
{|
Vbox
0,Seq
0,
Seq
Seq
Seq
Seq
Seq Nop,Verbatim "This text has a ",Tag
[ Bg_blue ],Verbatim
"blue background",
Verbatim
" but here it becomes the default background,",Verbatim
" even together with another background modifier." |}]
Seq Nop, Verbatim "This text has a ",
Tag [ Bg_blue ], Verbatim "blue background",
Verbatim " but here it becomes the default background,",
Verbatim " even together with another background modifier." |}]
;;

let%expect_test "parse 8-bit colors" =
Expand All @@ -263,7 +235,9 @@ let%expect_test "parse 8-bit colors" =
[%expect
{|
Vbox
0,Seq
0,
Seq
Seq
Seq
Seq
Seq
Expand All @@ -273,25 +247,18 @@ Vbox
Seq
Seq
Seq
Seq
Seq Nop,Verbatim "This is a ",Tag
[ Fg_8_bit_color 33
],Verbatim "blue",
Verbatim
" string with ",Tag
[ Fg_8_bit_color 196 ],Verbatim
"red",
Verbatim
" and ",Tag [ Fg_8_bit_color 46 ],Verbatim "green",
Verbatim
" together with strings of a ",Tag
[ Bg_8_bit_color 33 ],
Verbatim
"blue blackground",
Verbatim
" and ",Tag [ Bg_8_bit_color 196 ],Verbatim "red background",
Verbatim
" and ",Tag [ Bg_8_bit_color 46 ],Verbatim "green background" |}]
Seq Nop, Verbatim "This is a ",
Tag [ Fg_8_bit_color 33 ], Verbatim "blue",
Verbatim " string with ",
Tag [ Fg_8_bit_color 196 ], Verbatim "red",
Verbatim " and ",
Tag [ Fg_8_bit_color 46 ], Verbatim "green",
Verbatim " together with strings of a ",
Tag [ Bg_8_bit_color 33 ], Verbatim "blue blackground",
Verbatim " and ",
Tag [ Bg_8_bit_color 196 ], Verbatim "red background",
Verbatim " and ",
Tag [ Bg_8_bit_color 46 ], Verbatim "green background" |}]
;;

let%expect_test "parse 24-bit colors" =
Expand All @@ -308,7 +275,9 @@ let%expect_test "parse 24-bit colors" =
[%expect
{|
Vbox
0,Seq
0,
Seq
Seq
Seq
Seq
Seq
Expand All @@ -318,31 +287,21 @@ let%expect_test "parse 24-bit colors" =
Seq
Seq
Seq
Seq
Seq Nop,Verbatim "This is a ",Tag
[ Fg_24_bit_color
[ 255; 0; 0 ]
],Verbatim "blue",
Verbatim
" string with ",Tag
[ Fg_24_bit_color [ 0; 255; 0 ] ],
Verbatim
"red",Verbatim " and ",
Tag
[ Fg_24_bit_color [ 0; 0; 255 ] ],Verbatim "green",
Verbatim
" together with strings of a ",Tag
[ Bg_24_bit_color
[ 255; 0; 0 ]
],Verbatim
"blue blackground",
Verbatim
" and ",Tag
[ Bg_24_bit_color [ 0; 255; 0 ] ],Verbatim
"red background",
Verbatim
" and ",Tag
[ Bg_24_bit_color [ 0; 0; 255 ] ],Verbatim
"green background"
Seq Nop, Verbatim "This is a ",
Tag
[ Fg_24_bit_color [ 255; 0; 0 ] ],
Verbatim "blue",
Verbatim " string with ",
Tag [ Fg_24_bit_color [ 0; 255; 0 ] ], Verbatim "red",
Verbatim " and ",
Tag [ Fg_24_bit_color [ 0; 0; 255 ] ], Verbatim "green",
Verbatim " together with strings of a ",
Tag
[ Bg_24_bit_color [ 255; 0; 0 ] ],
Verbatim "blue blackground",
Verbatim " and ",
Tag [ Bg_24_bit_color [ 0; 255; 0 ] ], Verbatim "red background",
Verbatim " and ",
Tag [ Bg_24_bit_color [ 0; 0; 255 ] ], Verbatim "green background"
|}]
;;
Loading

0 comments on commit f6c4ad3

Please sign in to comment.