Improve and cleanup sequence_blank_line (fix empty_line_between) by Julow · Pull Request #1075 · ocaml-ppx/ocamlformat

Julow · 2019-10-14T09:57:55Z

This PR does:

Change the API of Source a little to avoid creating Location.t values.
I think we should only read those values, we should manipulate pairs of Lexing.position instead.
Use Lexing internals to set the starting location to avoid offsetting them.
Fix Source.empty_line_between to be true if there is at least one empty line instead of if all lines are empty.
Change sequence_blank_line to use Source.empty_line_between

gpetiot · 2019-10-15T05:01:16Z

It looks good to me (I detailed the changelog a bit more), but there is a tiny diff with test_branch on js_of_ocaml:

diff --git a/examples/graph_viewer/svg.ml b/examples/graph_viewer/svg.ml
index 036517813..d85d3f19c 100644
--- a/examples/graph_viewer/svg.ml
+++ b/examples/graph_viewer/svg.ml
@@ -557,6 +557,7 @@ let _ =
     ();
   let slider = GRange.scale `HORIZONTAL ~draw_value:false ~packing:b#pack () in
   slider#adjustment#set_bounds ~lower:0. ~upper:1. ~step_incr:0.1 ();
+
   (*
   let button = GButton.check_button ~label:"Animate"
       ~packing:b#pack () in
diff --git a/examples/planet/planet.ml b/examples/planet/planet.ml
index 1322aba6c..876a3db6e 100644
--- a/examples/planet/planet.ml
+++ b/examples/planet/planet.ml
@@ -520,6 +520,7 @@ let draw ctx _img shd o _uv normals face_info dir =
         let e = (dy2 *. du3) -. (dy3 *. du2) in
         let f = y1 -. (d *. u1) -. (e *. v1) in
         ctx##transform a d b e c f;
+
         (*
 let (u1, v1) = uv.(v1) in
 let (u2, v2) = uv.(v2) in

There is no blank line before the comments in the original files, but there are blank lines inside the comments. Shouldn't empty_line_between check that the empty line is at the beginning or at the end of the lines?

Julow · 2019-10-15T13:31:32Z

That's a tricky problem.
Checking only the first and last lines will not work in this case:

  (* This let will wrap *)
  let x =
    1
  in

  (* some comment *)
  next statement;

The solution may be to combine the previous algorithm that was looking at comments with Source.empty_line_between.

It was false if there was any character between p1 and p2, even if there also was an empty line

Julow · 2019-10-15T14:13:04Z

I added a fix for that.

gpetiot

test_branch with sequence-blank-line=preserve-one and module-item-spacing=preserve looks good, it can be merged when it's ready.

CHANGES: #### Changes + Set "conventional" as the default profile (ocaml-ppx/ocamlformat#1060) (Guillaume Petiot) This new profile is made to better match the most used style and is encouraged. To continue using the previous default, use `profile = ocamlformat` in your `.ocamlformat`. + CLI: Allow both values of boolean options (ocaml-ppx/ocamlformat#1062) (Jules Aguillon) Now, both `--opt` and --no-opt` are available on the CLI for any boolean option "opt". Previously, only one of them were available depending on the default value. + Auto mode for `break-string-literals` (ocaml-ppx/ocamlformat#1057) (Guillaume Petiot) `wrap`, `newlines` and `newlines-and-wrap` values of `break-string-literals` are removed. `auto` replaces them, it is equivalent to `newlines-and-wrap`. + Dock collection brackets (ocaml-ppx/ocamlformat#1014) (Guillaume Petiot) `after-and-docked` value of `break-separators` is removed and is replaced by a new `dock-collection-brackets` option. + Preserve `begin` and `end` keywords in if-then-else (ocaml-ppx/ocamlformat#978) (Jules Aguillon) Previously, `begin`/`end` keywords around if-then-else branches were turned into parentheses. #### New features + Give a hint when warning 50 is raised (ocaml-ppx/ocamlformat#1111) (Guillaume Petiot) + Add a message when a config value is removed (ocaml-ppx/ocamlformat#1089) (Etienne Millon) Explain what replaces removed options and avoid printing a parsing error. + Implement `sequence-blank-line=preserve-one` for let bindings (ocaml-ppx/ocamlformat#1077) (Jules Aguillon) Preserve a blank line after `let .. in` when `sequence-blank-line` set to `preserve-one`. Previously, only blank lines after `;` could be preserved. + Parse toplevel directives (ocaml-ppx/ocamlformat#1020) (Jules Aguillon) Allow `#directives` in `.ml` files. Previously, files containing a directive needed to be parsed as "use file". The "use file" mode is removed and `--use-file` is now the same as `--impl`. + Don't require `--name`, require kind, forbid `--inplace`, allow `--check`, make `--enable-outside-detected-project` implicit when reading from stdin (ocaml-ppx/ocamlformat#1018) (Guillaume Petiot) + Parse code in docstrings (ocaml-ppx/ocamlformat#941) (Guillaume Petiot) Format OCaml code in cinaps-style comments `(*$ code *)` and code blocks in documentation comments `(** {[ code ]} *)`. + Parse documentation comments with Odoc (ocaml-ppx/ocamlformat#721) (Jules Aguillon) Formatting of documentation comments is more robust and support newer Odoc syntaxes. Internally, Odoc replaces Octavius as the documentation parser. #### Bug fixes + Fix unstabilizing comments on assignments (ocaml-ppx/ocamlformat#1093) (Guillaume Petiot) + Fix the default value documentation for `max-indent` (ocaml-ppx/ocamlformat#1105) (Guillaume Petiot) + Fix closing parenthesis exceeding the margin in function application (ocaml-ppx/ocamlformat#1098) (Jules Aguillon) + Missing break before attributes of `Pmty_with` (ocaml-ppx/ocamlformat#1103) (Josh Berdine) + Fix closing quote exceeding the margin (ocaml-ppx/ocamlformat#1096) (Jules Aguillon) + Fix break before the closing bracket of collections (exceeding the margin) (ocaml-ppx/ocamlformat#1073) (Guillaume Petiot) + Fix precedence of Dot wrt Hash (ocaml-ppx/ocamlformat#1058) (Guillaume Petiot) + Fix break in variant type definition to not exceed the margin (ocaml-ppx/ocamlformat#1064) (Guillaume Petiot) + Fix newlines and indentation in toplevel extension points (ocaml-ppx/ocamlformat#1054) (Guillaume Petiot) + Fix placement of doc comments around extensions (ocaml-ppx/ocamlformat#1052) (Jules Aguillon) + Inline extensions that do not break (ocaml-ppx/ocamlformat#1050) (Guillaume Petiot) + Add missing cut before attributes in type declarations (ocaml-ppx/ocamlformat#1051) (Guillaume Petiot) + Fix alignment of cases (ocaml-ppx/ocamlformat#1046) (Guillaume Petiot) + Fix blank line after comments at the end of lists (ocaml-ppx/ocamlformat#1045) (Guillaume Petiot) + Fix indexing operators precedence (ocaml-ppx/ocamlformat#1039) (Jules Aguillon) + Fix dropped comment after infix op (ocaml-ppx/ocamlformat#1030) (Guillaume Petiot) + No newline if the input is empty (ocaml-ppx/ocamlformat#1031) (Guillaume Petiot) + Fix unstable comments around attributes (ocaml-ppx/ocamlformat#1029) (Guillaume Petiot) + Fix extra blank line in sequence (ocaml-ppx/ocamlformat#1021) (Jules Aguillon) + Check functor arguments when computing placement of doc comments (ocaml-ppx/ocamlformat#1013) (Jules Aguillon) + Fix indentation of labelled args (ocaml-ppx/ocamlformat#1006) (Guillaume Petiot) + Fix linebreak between or-cases with comments when `break-cases=all` (ocaml-ppx/ocamlformat#1002) (Guillaume Petiot) + Fix unstable unattached doc comment in records (ocaml-ppx/ocamlformat#998) (Jules Aguillon) + Fix string literal changed (ocaml-ppx/ocamlformat#995) (Jules Aguillon) + Fix type variable (ocaml-ppx/ocamlformat#996) (Jules Aguillon) + Fix crash on extension sequence (ocaml-ppx/ocamlformat#992) (Guillaume Petiot) + Fix position of expressions regarding of comments in infix-op expressions (ocaml-ppx/ocamlformat#986) (Guillaume Petiot) + Escape special characters in external declaration (ocaml-ppx/ocamlformat#988) (Jules Aguillon) + Fix parens around constrained expr with attrs (ocaml-ppx/ocamlformat#987) (Guillaume Petiot) + Fix the margin, and correctly breaks comments (ocaml-ppx/ocamlformat#957) (Guillaume Petiot) + Fix formatting of custom indexing operators (ocaml-ppx/ocamlformat#975) (Guillaume Petiot) + Fix position of comments of labelled arrow types (ocaml-ppx/ocamlformat#976) (Guillaume Petiot) + No box around inline odoc styles (ocaml-ppx/ocamlformat#971) (Guillaume Petiot) + Fix boxing of collection expressions/patterns (ocaml-ppx/ocamlformat#960) (Guillaume Petiot) + Fix crash on record expr with pack fields (ocaml-ppx/ocamlformat#963) (Jules Aguillon) + Fix letop in subexpr (ocaml-ppx/ocamlformat#956) (hhugo) #### Internal + Take file kind from --name when formatting stdin (ocaml-ppx/ocamlformat#1119) (Jules Aguillon) + Make Fmt.t abstract (ocaml-ppx/ocamlformat#1109) (Jules Aguillon) + Future-proof Fmt API in case Fmt.t goes abstract (ocaml-ppx/ocamlformat#1106) (Etienne Millon) + Future-proof `Fmt` API in case `Fmt.t` goes abstract (ocaml-ppx/ocamlformat#1106) (Etienne Millon) + Optional names for formatting boxes in debug output (ocaml-ppx/ocamlformat#1083) (Guillaume Petiot) + Check ocamlformat error codes in the testsuite (ocaml-ppx/ocamlformat#1084) (Etienne Millon) + Clean `Translation_unit` (ocaml-ppx/ocamlformat#1078) (Guillaume Petiot) + Use dune file generation in test/passing/dune (ocaml-ppx/ocamlformat#1082) (Etienne Millon) + CI: factorize tests and check reason build (ocaml-ppx/ocamlformat#1079) (Guillaume Petiot) + Use short form for action in src/dune (ocaml-ppx/ocamlformat#1076) (Etienne Millon) + Cleanup `sequence_blank_line` (ocaml-ppx/ocamlformat#1075) (Jules Aguillon) + CI: use a script travis-ci.sh to simplify .travis.yml (ocaml-ppx/ocamlformat#1063) (Guillaume Petiot) + Remove utility functions from `Fmt_ast` (ocaml-ppx/ocamlformat#1059) (Guillaume Petiot) + CI: use opam-2.0.5 in Travis (ocaml-ppx/ocamlformat#1044) (Anton Kochkov) + CI: check the build with OCaml 4.07.1 and 4.08.0 (ocaml-ppx/ocamlformat#1036) (Jules Aguillon) + Use the same sets of options for both branches by default in `test_branch.sh` (ocaml-ppx/ocamlformat#1033) (Guillaume Petiot) + Fix `test_branch.sh` and CI checking of CHANGES.md (ocaml-ppx/ocamlformat#1032, ocaml-ppx/ocamlformat#1034) (Jules Aguillon) + Fix flag of git-worktree in `test_branch.sh` and `bisect.sh` (ocaml-ppx/ocamlformat#1027) (Guillaume Petiot) + Remove the `bisect_ppx` dependency and clean the `Makefile` (ocaml-ppx/ocamlformat#1005) (Jules Aguillon) + Use a `CHANGES.md` log file again (ocaml-ppx/ocamlformat#1023) (Guillaume Petiot) + Support OCaml 4.09.0 (add the odoc.1.4.2 dependency) (ocaml-ppx/ocamlformat#1024) (Guillaume Petiot) + Update labels of issue templates (ocaml-ppx/ocamlformat#1017) (Guillaume Petiot) + Update labels in `CONTRIBUTING.md` (ocaml-ppx/ocamlformat#1007) (Guillaume Petiot) + Allow to ignore invalid options (ocaml-ppx/ocamlformat#984) (hhugo) The `--ignore-invalid-option` flag is added to ignore invalid options in `.ocamlformat` files. + Improve the documentation of `--doc-comments` (ocaml-ppx/ocamlformat#982) (Jules Aguillon) + Remove symbolic links and change naming convention of tests (ocaml-ppx/ocamlformat#980) (Guillaume Petiot) + Change the type of `fmt_code` (ocaml-ppx/ocamlformat#974) (Guillaume Petiot) + Simplify `Makefile` (ocaml-ppx/ocamlformat#973) (hhugo) + Dune should not be flagged as a build dep anymore (ocaml-ppx/ocamlformat#954) (Guillaume Petiot)

…ml-ppx#1075) * Remove some Location calculation and correctly setup Lexing * Use Lexing.position instead of Location.t in empty_line_between * Fix empty_line_between when there is non-empty line between p1 and p2 * Implement sequence_blank_line using Source.empty_line_between * Fix sequence_blank_line when there is empty lines in comments

Julow requested a review from gpetiot October 14, 2019 09:58

facebook-github-bot added the CLA Signed label Oct 14, 2019

Julow mentioned this pull request Oct 14, 2019

Improve: implement sequence-blank-line=preserve-one for let bindings #1077

Merged

gpetiot force-pushed the fix-blank-line branch from 24a03e0 to 07d1893 Compare October 15, 2019 04:39

gpetiot changed the title ~~Cleanup sequence_blank_line~~ Improve and cleanup sequence_blank_line (fix empty_line_between) Oct 15, 2019

gpetiot mentioned this pull request Oct 15, 2019

Improve: extend option sequence-blank-line to let-bindings #1056

Closed

Julow force-pushed the fix-blank-line branch from 1328adc to bde2f0b Compare October 15, 2019 13:55

Julow added 6 commits October 15, 2019 15:56

Add tests

8531807

Remove some Location calculation and correctly setup Lexing

2d0a19a

Use Lexing.position instead of Location.t in empty_line_between

1faab70

Fix empty_line_between when there is non-empty line between p1 and p2

6ea1991

It was false if there was any character between p1 and p2, even if there also was an empty line

Implement sequence_blank_line using Source.empty_line_between

0d27d8b

Update CHANGES.md

9e206cd

Julow force-pushed the fix-blank-line branch 2 times, most recently from af0251a to 09df237 Compare October 15, 2019 14:12

Julow added 2 commits October 15, 2019 16:13

Add test for empty line in comment

4653776

Fix sequence_blank_line when there is empty lines in comments

09df237

gpetiot approved these changes Oct 15, 2019

View reviewed changes

Julow merged commit 58b7e1d into ocaml-ppx:master Oct 15, 2019

Julow mentioned this pull request Nov 4, 2019

[new release] ocamlformat (0.12) ocaml/opam-repository#15208

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve and cleanup sequence_blank_line (fix empty_line_between)#1075

Improve and cleanup sequence_blank_line (fix empty_line_between)#1075
Julow merged 8 commits intoocaml-ppx:masterfrom
Julow:fix-blank-line

Julow commented Oct 14, 2019

Uh oh!

gpetiot commented Oct 15, 2019 •

edited

Loading

Uh oh!

Julow commented Oct 15, 2019

Uh oh!

Julow commented Oct 15, 2019

Uh oh!

gpetiot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Julow commented Oct 14, 2019

Uh oh!

gpetiot commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Julow commented Oct 15, 2019

Uh oh!

Julow commented Oct 15, 2019

Uh oh!

gpetiot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gpetiot commented Oct 15, 2019 •

edited

Loading