Skip to content

Fix: adjust definition of Location.is_single_line to reflect margin#1102

Merged
gpetiot merged 3 commits intomasterfrom
1
Jan 24, 2020
Merged

Fix: adjust definition of Location.is_single_line to reflect margin#1102
gpetiot merged 3 commits intomasterfrom
1

Conversation

@jberdine
Copy link
Copy Markdown
Collaborator

@jberdine jberdine commented Oct 27, 2019

This seems to be needed to be consistent with recent changes to the
handling of the margin. Otherwise, it is possible to have a one-liner
structure item of just the right width that is_single_line does not
hold, resulting in the one-liner being surrounded by open lines.

@gpetiot
Copy link
Copy Markdown
Collaborator

gpetiot commented Oct 28, 2019

Otherwise, it is possible to have a one-liner
structure item of just the right width that is_single_line does not
hold, resulting in the one-liner being surrounded by open lines.

Do you have an example for this?

@jberdine
Copy link
Copy Markdown
Collaborator Author

jberdine commented Oct 30, 2019

Yes, IIRC one is:

val xx_xxxxxxxx : t -> bool
val xxxxxxxx : t -> [> `Xxxxxx | `Xxxxxxxxxxx | `Xxxxxxxxxx | `Xxxxxxxxxxxxx]
val xxxxx : t -> t -> t Xxx.t option

with a margin of 77.

@gpetiot
Copy link
Copy Markdown
Collaborator

gpetiot commented Nov 6, 2019

I tested it on master, the output of is_single_line is true for each item of this file.

@jberdine
Copy link
Copy Markdown
Collaborator Author

jberdine commented Nov 12, 2019

It seems that module-item-spacing=compact is also needed. I added a test file that should be unchanged, and is with this PR, but isn't on master.

@gpetiot
Copy link
Copy Markdown
Collaborator

gpetiot commented Nov 18, 2019

val xxxxxxxx : t -> [> Xxxxxxx | Xxxxxxxxxxx | Xxxxxxxxxx | Xxxxxxxxxxxxx] is 88 characters long but it doesn't break, like #1069 (square brackets), we can start by fixing this.

@gpetiot
Copy link
Copy Markdown
Collaborator

gpetiot commented Jan 24, 2020 •
edited
Loading

I rebased the branch, there is no diff with test_branch except with the janestreet profile:

diff --git a/compiler/num-testsuite/test_big_ints.ml b/compiler/num-testsuite/test_big_ints.ml
index c2390ae3a..2475eb912 100644
--- a/compiler/num-testsuite/test_big_ints.ml
+++ b/compiler/num-testsuite/test_big_ints.ml
@@ -103,10 +103,8 @@ test
 
 ;;
 test 16 eq_big_int (add_big_int (big_int_of_int (-1)) (big_int_of_int 2), big_int_of_int 1)
-
 ;;
 test 17 eq_big_int (add_big_int (big_int_of_int 2) (big_int_of_int (-1)), big_int_of_int 1)
-
 ;;
 testing_function "sub_big_int"
 ;;
@@ -166,7 +164,6 @@ test
 
 ;;
 test 17 eq_big_int (sub_big_int (big_int_of_int 2) (big_int_of_int (-1)), big_int_of_int 3)
-
 ;;
 testing_function "mult_int_big_int"
 ;;
@@ -946,7 +943,6 @@ test
 testing_function "shift_right_big_int"
 ;;
 test 1 eq_big_int (shift_right_big_int unit_big_int 0, unit_big_int)
-
 ;;
 test 2 eq_big_int (shift_right_big_int (big_int_of_int 12345678) 3, big_int_of_int 1543209)
 
diff --git a/compiler/num-testsuite/test_nums.ml b/compiler/num-testsuite/test_nums.ml
index d4fc822a6..469841bcf 100644
--- a/compiler/num-testsuite/test_nums.ml
+++ b/compiler/num-testsuite/test_nums.ml
@@ -97,7 +97,6 @@ test 10 eq_num (sub_num (Int (-1)) (Int biggest_int), pred_num (Int least_int))
 testing_function "mult_num"
 ;;
 test 1 eq_num (mult_num (Int 2) (Int 3), Int 6)
-
 ;;
 test 2 eq_num (mult_num (Int 127) (Int (int_of_string "257")), Int (int_of_string "32639"))
 
diff --git a/compiler/num-testsuite/test_ratios.ml b/compiler/num-testsuite/test_ratios.ml
index 32a8fa7f5..40d53626a 100644
--- a/compiler/num-testsuite/test_ratios.ml
+++ b/compiler/num-testsuite/test_ratios.ml
@@ -74,10 +74,8 @@ test 9 eq_big_int (numerator_ratio r, big_int_of_int 0)
 
 ;;
 testing_function "null_denominator"
-
 ;;
 test 1 eq (null_denominator (create_ratio (big_int_of_int 1) (big_int_of_int (-2))), false)
-
 ;;
 test 2 eq (null_denominator (create_ratio (big_int_of_int 1) zero_big_int), true)

The longer lines of this diff exceed the 90 column margin, but it was already the case in master (related to #1066, #1069). The new output is more consistent since it behaves the same way for every non-breaking lines.
So maybe we can merge it afterall @Julow ?

@gpetiot gpetiot requested a review from Julow January 24, 2020 11:14
Julow
Julow reviewed Jan 24, 2020
View reviewed changes
jberdine and others added 3 commits January 24, 2020 22:05
@jberdine @gpetiot
Adjust definition of Location.is_single_line to reflect margin
43ed876 
This seems to be needed to be consistent with recent changes to the
handling of the margin. Otherwise, it is possible to have a one-liner
structure item of just the right width that is_single_line does not
hold, resulting in the one-liner being surrounded by open lines.
@gpetiot
changelog
5da9daa
@gpetiot
add comment
8ee2f4e
Julow
Julow approved these changes Jan 24, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@gpetiot gpetiot changed the title Improve: Adjust definition of Location.is_single_line to reflect margin Fix: adjust definition of Location.is_single_line to reflect margin Jan 24, 2020
@gpetiot gpetiot merged commit 1cdac88 into master Jan 24, 2020
@gpetiot gpetiot deleted the 1 branch January 24, 2020 16:28
@Julow Julow mentioned this pull request Jan 28, 2020
Merged
Julow added a commit to Julow/opam-repository that referenced this pull request Jan 28, 2020
@Julow
[new release] ocamlformat (0.13.0)
0363fa8 
CHANGES:

#### New features

  + Add an option `--margin-check` to emit a warning if the formatted output exceeds the margin (ocaml-ppx/ocamlformat#1110) (Guillaume Petiot)
  + Preserve comment indentation when `wrap-comments` is unset (ocaml-ppx/ocamlformat#1138, ocaml-ppx/ocamlformat#1159) (Jules Aguillon)
  + Improve error messages (ocaml-ppx/ocamlformat#1147) (Jules Aguillon)
  + Display standard output in the emacs plugin even when ocamlformat does not fail (ocaml-ppx/ocamlformat#1189) (Guillaume Petiot)

#### Removed

  + Remove `ocamlformat_reason` (ocaml-ppx/ocamlformat#254, ocaml-ppx/ocamlformat#1185) (Etienne Millon).
    This tool has never been released to opam, has no known users, and overlaps
    with what `refmt` can do.
  + Remove `ocamlformat-diff` (ocaml-ppx/ocamlformat#1205) (Guillaume Petiot)
    This tool has never been released to opam, has no known users, and overlaps
    with what `merge-fmt` can do.

#### Packaging

  + Work with base v0.13.0 (ocaml-ppx/ocamlformat#1163) (Jules Aguillon)

#### Bug fixes

  + Fix placement of comments just before a '|' (ocaml-ppx/ocamlformat#1203) (Jules Aguillon)
  + Fix build version detection when building in the absence of a git root (ocaml-ppx/ocamlformat#1198) (Anil Madhavapeddy)
  + Fix wrapping of or-patterns in presence of comments with `break-cases=fit` (ocaml-ppx/ocamlformat#1167) (Jules Aguillon)
    This also fixes an unstable comment bug in or-patterns
  + Fix an unstable comment bug in variant declarations (ocaml-ppx/ocamlformat#1108) (Jules Aguillon)
  + Fix: break multiline comments (ocaml-ppx/ocamlformat#1122) (Guillaume Petiot)
  + Fix: types on named arguments were wrapped incorrectly when preceding comments (ocaml-ppx/ocamlformat#1124) (Guillaume Petiot)
  + Fix the indentation produced by max-indent (ocaml-ppx/ocamlformat#1118) (Guillaume Petiot)
  + Fix break after Psig_include depending on presence of docstring (ocaml-ppx/ocamlformat#1125) (Guillaume Petiot)
  + Remove some calls to if_newline and break_unless_newline and fix break before closing brackets (ocaml-ppx/ocamlformat#1168) (Guillaume Petiot)
  + Fix unstable cmt in or-pattern (ocaml-ppx/ocamlformat#1173) (Guillaume Petiot)
  + Fix location of comment attached to the underscore of an open record (ocaml-ppx/ocamlformat#1208) (Guillaume Petiot)
  + Fix parentheses around optional module parameter (ocaml-ppx/ocamlformat#1212) (Christian Barcenas)
  + Fix grouping of horizontally aligned comments (ocaml-ppx/ocamlformat#1209) (Guillaume Petiot)
  + Fix dropped comments around module pack expressions (ocaml-ppx/ocamlformat#1214) (Jules Aguillon)
  + Fix regression of comment position in list patterns (ocaml-ppx/ocamlformat#1141) (Josh Berdine)
  + Fix: adjust definition of Location.is_single_line to reflect margin (ocaml-ppx/ocamlformat#1102) (Josh Berdine)

#### Documentation

  + Fix documentation of option `version-check` (ocaml-ppx/ocamlformat#1135) (Wilfred Hughes)
  + Fix hint when using `break-separators=after-and-docked` (ocaml-ppx/ocamlformat#1130) (Greta Yorsh)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpetiot gpetiot gpetiot approved these changes

@Julow Julow Julow approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jberdine @gpetiot @Julow @facebook-github-bot