-
Notifications
You must be signed in to change notification settings - Fork 38
Callbacks without exponential explosion. #590
Conversation
->Js.Dict.get("wm-property") | ||
->Option.flatMap(Js.Json.decodeString) | ||
->Option.flatMap(x => | ||
let b = x->Js.Dict.get("wm-property")->Option.flatMap(Js.Json.decodeString)->Option.flatMap(x => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIKT this would be a big regression. The community loves tidy pipe chains on multiple lines after a certain line width has been reached.
But I guess this causes most of the perf issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no causality.
It's an unintended effect of how the exponential explosion is avoided.
{possibleGradeValues | ||
|> List.filter(g => g <= state.maxGrade) | ||
|> List.map(possibleGradeValue => | ||
<div> {possibleGradeValues |> List.filter(g => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous version was more readable unfortunately
), | ||
) | ||
}) | ||
library.getBalance(. account)->Promise.Js.catch(_ => {Promise.resolved(None)})->Promise.get( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Promises really profit from per-line pipe formatting. But I guess this might not even be relevant anymore for the "next big feature"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems indeed like a "regression" compared to the previous formatted code
{possibleGradeValues | ||
|> List.filter(g => g < state.maxGrade) | ||
|> List.map(possibleGradeValue => | ||
<div> {possibleGradeValues |> List.filter(g => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code seems to be printed in a pretty hard to read way.
How hard is it to preserve the previous format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea! But I have a different approach in mind that should preserve pretty much all the formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that it's unclear why the examples have changed, is more worrying than the changes themselves.
possibilities->Belt.Array.mapU((. combination) => | ||
combination->Belt.Array.reduceU(Nil, (. tree, curr) => tree->insert(curr)) | ||
) | ||
let trees = possibilities->Belt.Array.mapU((. combination) => combination->Belt.Array.reduceU( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep the formatting on two lines?
combination->Belt.Array.reduceU(Nil, (. tree, curr) => tree->insert(curr)) | ||
) | ||
) { | ||
if possibilities->Belt.Array.mapU((. combination) => combination->Belt.Array.reduceU(Nil, (. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this break?
@@ -180,17 +179,13 @@ foo(list => list()) | |||
foo(\"switch" => \"switch"()) | |||
|
|||
// the [] of the array should break | |||
[ | |||
fn(x => { | |||
[fn(x => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda prefer the previous formatting.
Follows #576