Skip to content
This repository has been archived by the owner on Jun 15, 2023. It is now read-only.

Commit

Permalink
Improve printing of non-callback arguments in Pexp_apply with callbac…
Browse files Browse the repository at this point in the history 
…ks (#241)

Fix #212

Sometimes one of the non-callback arguments will break in a function application where the first or last argument is a callback. There might be a single line comment in there, or a multiline string. We want to break all the arguments in this case for readability.

**before**
```rescript
showDialog(
  `
  Do you really want to leave this workspace?
  Some more text with detailed explanations...
  `, danger=true, ~confirmText="Yes, I am sure!", ~onConfirm={() => ()},
)
```

**after**
```rescript
showDialog(
  `
  Do you really want to leave this workspace?
  Some more text with detailed explanations...
  `,
  ~danger=true,
  ~confirmText="Yes, I am sure!",
  ~onConfirm={() => ()},
)
```
  • Loading branch information
@IwanKaramazow
IwanKaramazow authored Jan 19, 2021
1 parent 110d9c6 commit 7415b09
Show file tree
Hide file tree
Showing 4 changed files with 211 additions and 23 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
## Unreleased

* Improve printing of non-callback arguments in call expressions with callback in [#241](https://github.com/rescript-lang/syntax/pull/241/files)
* Fix printing of constrained expressions in rhs of js objects [#240](https://github.com/rescript-lang/syntax/pull/240)
* Improve printing of trailing comments under lhs of "pipe" expression in [#329](https://github.com/rescript-lang/syntax/pull/239/files)
* Improve printing of jsx children and props with leading line comment in [#236](https://github.com/rescript-lang/syntax/pull/236)
Expand Down
89 changes: 66 additions & 23 deletions src/res_printer.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3991,6 +3991,7 @@ and printArgumentsWithCallbackInFirstPosition ~uncurried args cmtTbl =
lblDoc;
printPexpFun ~inCallback:FitsOnOneLine expr cmtTbl
] in
let callback = printComments callback cmtTbl expr.pexp_loc in
let printedArgs =
Doc.join ~sep:(Doc.concat [Doc.comma; Doc.line]) (
List.map (fun arg -> printArgument arg cmtTbl) args
Expand All @@ -3999,6 +4000,7 @@ and printArgumentsWithCallbackInFirstPosition ~uncurried args cmtTbl =
(callback, printedArgs)
| _ -> assert false
in

(* Thing.map((arg1, arg2) => MyModuleBlah.toList(argument), foo) *)
(* Thing.map((arg1, arg2) => {
* MyModuleBlah.toList(argument)
Expand All @@ -4021,10 +4023,29 @@ and printArgumentsWithCallbackInFirstPosition ~uncurried args cmtTbl =
* )
*)
let breakAllArgs = printArguments ~uncurried args cmtTblCopy in
Doc.customLayout [
fitsOnOneLine;
breakAllArgs;
]

(* Sometimes one of the non-callback arguments will break.
* There might be a single line comment in there, or a multiline string etc.
* showDialog(
* ~onConfirm={() => ()},
* `
* Do you really want to leave this workspace?
* Some more text with detailed explanations...
* `,
* ~danger=true,
* // comment --> here a single line comment
* ~confirmText="Yes, I am sure!",
* )
* In this case, we always want the arguments broken over multiple lines,
* like a normal function call.
*)
if Doc.willBreak printedArgs then
breakAllArgs
else
Doc.customLayout [
fitsOnOneLine;
breakAllArgs;
]

and printArgumentsWithCallbackInLastPosition ~uncurried args cmtTbl =
(* Because the same subtree gets printed twice, we need to copy the cmtTbl.
Expand All @@ -4047,13 +4068,19 @@ and printArgumentsWithCallbackInLastPosition ~uncurried args cmtTbl =
]
in
let callbackFitsOnOneLine =
printPexpFun ~inCallback:FitsOnOneLine expr cmtTbl in
let callbackArgumetnsFitsOnOneLine =
printPexpFun ~inCallback:ArgumentsFitOnOneLine expr cmtTblCopy in
let pexpFunDoc = printPexpFun ~inCallback:FitsOnOneLine expr cmtTbl in
let doc = Doc.concat [lblDoc; pexpFunDoc] in
printComments doc cmtTbl expr.pexp_loc
in
let callbackArgumentsFitsOnOneLine =
let pexpFunDoc = printPexpFun ~inCallback:ArgumentsFitOnOneLine expr cmtTblCopy in
let doc = Doc.concat [lblDoc; pexpFunDoc] in
printComments doc cmtTblCopy expr.pexp_loc
in
(
Doc.concat (List.rev acc),
Doc.concat [lblDoc; callbackFitsOnOneLine],
Doc.concat [lblDoc; callbackArgumetnsFitsOnOneLine]
callbackFitsOnOneLine,
callbackArgumentsFitsOnOneLine
)
| arg::args ->
let argDoc = printArgument arg cmtTbl in
Expand Down Expand Up @@ -4090,11 +4117,30 @@ and printArgumentsWithCallbackInLastPosition ~uncurried args cmtTbl =
* )
*)
let breakAllArgs = printArguments ~uncurried args cmtTblCopy2 in
Doc.customLayout [
fitsOnOneLine;
arugmentsFitOnOneLine;
breakAllArgs;
]

(* Sometimes one of the non-callback arguments will break.
* There might be a single line comment in there, or a multiline string etc.
* showDialog(
* `
* Do you really want to leave this workspace?
* Some more text with detailed explanations...
* `,
* ~danger=true,
* // comment --> here a single line comment
* ~confirmText="Yes, I am sure!",
* ~onConfirm={() => ()},
* )
* In this case, we always want the arguments broken over multiple lines,
* like a normal function call.
*)
if Doc.willBreak printedArgs then
breakAllArgs
else
Doc.customLayout [
fitsOnOneLine;
arugmentsFitOnOneLine;
breakAllArgs;
]

and printArguments ~uncurried (args : (Asttypes.arg_label * Parsetree.expression) list) cmtTbl =
match args with
Expand Down Expand Up @@ -4357,23 +4403,20 @@ and printExprFunParameters ~inCallback ~uncurried ~hasConstraint parameters cmtT
let printedParamaters = Doc.concat [
if shouldHug || inCallback then Doc.nil else Doc.softLine;
Doc.join
~sep:(
Doc.concat [
Doc.comma; if inCallback then Doc.line else Doc.line
]
)
~sep:(Doc.concat [Doc.comma; Doc.line])
(List.map (fun p -> printExpFunParameter p cmtTbl) parameters)
] in
Doc.group (
Doc.concat [
lparen;
if shouldHug || inCallback then
printedParamaters
else Doc.indent printedParamaters;
if shouldHug || inCallback then
Doc.nil
else
Doc.concat [Doc.trailingComma; Doc.softLine];
Doc.concat [
Doc.indent printedParamaters;
Doc.trailingComma;
Doc.softLine;
];
Doc.rparen;
]
)
Expand Down
75 changes: 75 additions & 0 deletions tests/printer/expr/__snapshots__/render.spec.js.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1580,6 +1580,81 @@ let /* a */ decoratorTags /* b */ = items->Js.Array2.filter(items => {
items.category === ChristmasLighting ||
items.category === Unknown
})
// callback in last position
showDialog(
\`
Do you really want to leave this workspace?
Some more text with detailed explanations...
\`,
~danger=true,
~confirmText=\\"Yes, I am sure!\\",
~onConfirm={() => ()},
)
// callback in first position
showDialog(
~onConfirm={() => ()},
~danger=true,
~confirmText=\\"Yes, I am sure!\\",
\`
Do you really want to leave this workspace?
Some more text with detailed explanations...
\`,
)
// callback in last position, with comment in between args
showDialog(
dialogMessage,
// comment
~danger=true,
~confirmText=\\"Yes, I am sure!\\",
~onConfirm=() => (),
)
// callback in last position, with comment in between args
showDialog(
dialogMessage,
~danger=true,
/* comment below */
~confirmText=\\"Yes, I am sure!\\",
~onConfirm=() => (),
)
// callback in first position, with single line comment in between args
showDialog(
~onConfirm=() => (),
dialogMessage,
// comment
~danger=true,
~confirmText=\\"Yes, I am sure!\\",
)
// callback in first position, with comment in between args
showDialog(
~onConfirm=() => (),
dialogMessage,
~danger=true,
/* comment below */
~confirmText=\\"Yes, I am sure!\\",
)
React.useEffect5(
(
context.activate,
context.chainId,
dispatch,
setTriedLoginAlready,
triedLoginAlready,
),
() => {
doThings()
None
}, // intentionally only running on mount (make sure it's only mounted once :))
)
apply(a, b, c, /* before */ () => () /* after */)
apply(/* before */ () => () /* after */, a, b, c)
"
`;
Expand Down
69 changes: 69 additions & 0 deletions tests/printer/expr/callback.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -319,3 +319,72 @@ let /* a */ decoratorTags /* b */ = items->Js.Array2.filter(items => {
|| items.category === ChristmasLighting
|| items.category === Unknown
})

// callback in last position
showDialog(
`
Do you really want to leave this workspace?
Some more text with detailed explanations...
`,
~danger=true,
~confirmText="Yes, I am sure!",
~onConfirm={() => ()},
)

// callback in first position
showDialog(
~onConfirm={() => ()},
~danger=true,
~confirmText="Yes, I am sure!",
`
Do you really want to leave this workspace?
Some more text with detailed explanations...
`,
)

// callback in last position, with comment in between args
showDialog(dialogMessage,
// comment
~danger=true, ~confirmText="Yes, I am sure!", ~onConfirm=() => ())

// callback in last position, with comment in between args
showDialog(
dialogMessage,
~danger=true,
/* comment below */
~confirmText="Yes, I am sure!",
~onConfirm=() => ())

// callback in first position, with single line comment in between args
showDialog(
~onConfirm=() => (),
dialogMessage,
// comment
~danger=true,
~confirmText="Yes, I am sure!"
)

// callback in first position, with comment in between args
showDialog(
~onConfirm=() => (),
dialogMessage,
~danger=true,
/* comment below */
~confirmText="Yes, I am sure!",
)

React.useEffect5((
context.activate,
context.chainId,
dispatch,
setTriedLoginAlready,
triedLoginAlready,
),
() => {
doThings()
None
}, // intentionally only running on mount (make sure it's only mounted once :))
)

apply(a, b, c, /* before */ () => () /* after */)
apply(/* before */ () => () /* after */, a, b, c)

0 comments on commit 7415b09

Please sign in to comment.