Fix position of expressions regarding of comments in infix-op expressions#970
Fix position of expressions regarding of comments in infix-op expressions#970gpetiot wants to merge 26 commits intoocaml-ppx:masterfrom gpetiot:fmt-args
Conversation
|
Hmm... I'm getting this behaviour now: Which is perhaps not ideal either? The indentation on the line after the infix operators looks off to me. |
This reverts commit f669a0a.
|
I think it is fixed and ready to be reviewed now, here is the diff with test_branch: (this was before doing the same for `|> and other short operators, now the diff is obviously longer but still looks good) diff --git a/infer/src/biabduction/interproc.ml b/infer/src/biabduction/interproc.ml
index 0e2dff723..2d6baa9db 100644
--- a/infer/src/biabduction/interproc.ml
+++ b/infer/src/biabduction/interproc.ml
@@ -707,8 +707,9 @@ let prop_init_formals_seed tenv new_formals (prop : 'a Prop.t) : Prop.exposed Pr
List.map ~f:do_formal new_formals
in
let sigma_seed =
- create_seed_vars ((* formals already there plus new ones *)
- prop.Prop.sigma @ sigma_new_formals)
+ create_seed_vars
+ ( (* formals already there plus new ones *)
+ prop.Prop.sigma @ sigma_new_formals )
in
let sigma = sigma_seed @ sigma_new_formals in
let new_pi = prop.Prop.pi in
diff --git a/infer/src/clang/ClangWrapper.ml b/infer/src/clang/ClangWrapper.ml
index 450b693b3..234ccdcbd 100644
--- a/infer/src/clang/ClangWrapper.ml
+++ b/infer/src/clang/ClangWrapper.ml
@@ -89,8 +89,9 @@ let clang_driver_action_items : ClangCommand.t -> action_item list =
CanonicalCommand
( (* massage line to remove edge-cases for splitting *)
match
- "\"" ^ line ^ " \"" |> (* split by whitespace *)
- Str.split (Str.regexp_string "\" \"")
+ "\"" ^ line ^ " \""
+ |> (* split by whitespace *)
+ Str.split (Str.regexp_string "\" \"")
with
| prog :: args ->
ClangCommand.mk ~is_driver:false ClangQuotes.EscapedDoubleQuotes ~prog ~args
diff --git a/compiler/lib/generate.ml b/compiler/lib/generate.ml
index 728cf7e7b..22c886acf 100644
--- a/compiler/lib/generate.ml
+++ b/compiler/lib/generate.ml
@@ -1835,9 +1835,9 @@ let generate_shared_value ctx =
| None -> []
| Some v ->
[J.V v, Some (J.EDot (s_var Constant.global_object, "jsoo_runtime"), J.N)])
- @ List.map
- (StringMap.bindings ctx.Ctx.share.Share.vars.Share.strings)
- ~f:(fun (s, v) -> v, Some (str_js s, J.N))
+ @ List.map
+ (StringMap.bindings ctx.Ctx.share.Share.vars.Share.strings)
+ ~f:(fun (s, v) -> v, Some (str_js s, J.N))
@ List.map
(StringMap.bindings ctx.Ctx.share.Share.vars.Share.prims)
~f:(fun (s, v) -> v, Some (runtime_fun ctx s, J.N)))) |
src/Ast.ml
Outdated
| true | ||
| | _ -> false | ||
|
|
||
| let is_monadic_last_char = function '=' | '|' -> true | _ -> false |
There was a problem hiding this comment.
I'm not familiar with the existing code but this makes me uneasy; what about |>?
There was a problem hiding this comment.
at first I wanted to avoid doing it for |> because it makes a long diff but it is more consistent indeed, fixed with my last commit.
| include Requires_sub_terms | ||
|
|
||
| let is_monadic_expr e = | ||
| match prec_ast (Exp e) with Some InfixOp0 -> true | _ -> false |
There was a problem hiding this comment.
This causes a break just after |> operators
- |> List.sort
- ~compare:(Comparable.lift Location.compare_start ~f:Cmt.loc)
+ |>
+ List.sort ~compare:(Comparable.lift Location.compare_start ~f:Cmt.loc)There was a problem hiding this comment.
Yes, that is the new behavior for all these operators, that's why I was filtering a bit (size > 2, checking the last character), so should |> be treated like the rest or not?
I don't really have an opinion on that, and @craigfe has a diverging opinion, so if the both of you could agree, that would be great :D
There was a problem hiding this comment.
FWIW, I also think this break looks odd. My toy example was using infix operators at the end of each line, a la:
(* Open the repo *)
initialise >>=
(* Perform a subsequent action *)
subsequent_action >|=but when I'm writing this sort of code personally, I keep the infix operators at the start:
x
>>= foo
>>= bar(or the same with |>). I don't really mind which of the two OCamlformat chooses to pick (this is probably a reasonably important decision), but I do care about its interaction with comments. i.e., this looks wrong to me:
initialise
>>= (* Perform a subsequent action *)
subsequent_action
>|= (* Keep going... *)
another_actionAnd if OCamlformat chooses to enforce one or the other, there is the question of what to do with input flies that are doing it the other way (comments before/after the infix operator).
There was a problem hiding this comment.
(this may be an instance where it's worth having an option to select between prepending or appending infix operators)
There was a problem hiding this comment.
Yes that would be good to have that but it requires a bigger rewrite of the code, the sequences of infix operations are currently sugared into a weird structure that mix different operators (as long as they have the same precedence, e.g. >>= and >|=), so it's not as simple as it was for sequences of items separated by ; only.
|
I don't mind the indentation in initialise
>>= (* Perform a subsequent action *)
subsequent_action
>|= (* Keep going... *)
another_actionMultiline expressions will render the same and here the first comment is part of the I don't think it's worth adding a special case that move the operator at the end of the line. It will probably be considered as a bug by users. Comments could be moved before instead: initialise
(* Perform a subsequent action *)
>>= subsequent_action
(* Keep going... *)
>|= another_action(which does not work currently: if the whole expression fit, the comment will not be on its line) |
|
Replaced by #986 |
Improve this part of the code before working on #961