Open dsyme opened 2 years ago
This needs some discussion per @nojaf's comments above
Can we detect imperative code from an AST point of view and should we act upon that?
Yes, to some extent. At least we can detect when in the first part of a sequential, which is by far the most common case. This would then mean
let someFunction() =
if expr then expr else expr
1+1
could always suppress single-line if-then-else and format as:
let someFunction() =
if expr then
expr
else
expr
1+1
It would not apply in this case:
let someFunction() =
if expr then expr else expr
but that will be relatively rare for short if-then-else. (We would also have the option of considering this to be imperative/control-flow in all cases)
However either way would be an imperfect implementation of "Use multi-line formatting for imperative code" (assuming that is what we add to the style guide).
At least we can detect when in the first part of a sequential
Oh boy 🙈, this is easier said than done.Sequential
, LetOrUse
and LetOrUseBang
are often captured together as a series of expressions via an active pattern in SourceParser
.
Touching those parts might be quite tricky to always get right because it will probably depend on where you are coming from in CodePrinter
code. In the past, ASTContext
was used to indicate such things, and ASTContext
is pure evil. Anyway, the beauty of genExpr
is that it will have the same behaviour in all cases it is being invoked. Adding more logic on top of that depending on the parent expression will not be an easy task. I'd rather start with something smaller in scope.
this is easier said than done
How about just defaulting fsharp_max_if_then_else_short_width to 0 (and also in the style guide). A pragmatic solution that would require much less work and not cause much harm (IMO any single-line if/if-else block is unreadable).
@dsyme looking at https://github.com/fsharp/fslang-design/issues/646#issuecomment-1138808449.
The suggestion is there to move the ifExpr
outside of the setting.
Would it be a good start to change what we have today to:
fsharp_max_if_then_short_width
, counting the length of the thenExpr
and not allowing any nested SynExpr.IfThenElse
fsharp_max_if_then_else_short_width
, counting the length from the thenExpr
to the end of the elseExpr
Where would this current guidance fall into:
Do both settings have no effect on this? And we format this compactly if each line elif x then y
fits on one line?
I propose we add
fsharp_max_if_then_short_width
. Style guide update TBDSlack conversation with @nojaf:
Pros and Cons
The advantages of making this adjustment to Fantomas are more explicit formatting of some imperative code
The disadvantages of making this adjustment to Fantomas are not all cases of imperative code are covered
Examples
Please provide multiple examples (before and after scenarios) and explain what rules did apply to achieve the outcome.
Extra information
Estimated cost (XS, S, M, L, XL, XXL): S
Related suggestions: (put links to related suggestions here)
Affidavit (please submit!)
Please tick this by placing a cross in the box:
Please tick all that apply: