From f47c1ce2bcdd7ca278df36afc45781822e4d2c98 Mon Sep 17 00:00:00 2001 From: Iwan Date: Tue, 19 Jan 2021 21:09:47 +0100 Subject: [PATCH] Improve printing of non-callback arguments in Pexp_apply with callbacks Fix https://github.com/rescript-lang/syntax/issues/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={() => ()}, ) ``` --- CHANGELOG.md | 1 + src/res_printer.ml | 89 ++++++++++++++----- .../expr/__snapshots__/render.spec.js.snap | 75 ++++++++++++++++ tests/printer/expr/callback.js | 69 ++++++++++++++ 4 files changed, 211 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f466518..e437aed6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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) diff --git a/src/res_printer.ml b/src/res_printer.ml index ce5b5f71..e644553b 100644 --- a/src/res_printer.ml +++ b/src/res_printer.ml @@ -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 @@ -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) @@ -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. @@ -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 @@ -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 @@ -4357,11 +4403,7 @@ 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 ( @@ -4369,11 +4411,12 @@ and printExprFunParameters ~inCallback ~uncurried ~hasConstraint parameters cmtT 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; ] ) diff --git a/tests/printer/expr/__snapshots__/render.spec.js.snap b/tests/printer/expr/__snapshots__/render.spec.js.snap index d897df6c..e9ccc545 100644 --- a/tests/printer/expr/__snapshots__/render.spec.js.snap +++ b/tests/printer/expr/__snapshots__/render.spec.js.snap @@ -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) " `; diff --git a/tests/printer/expr/callback.js b/tests/printer/expr/callback.js index a3154093..4a3dfc67 100644 --- a/tests/printer/expr/callback.js +++ b/tests/printer/expr/callback.js @@ -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)