fsharp / fsharp/fslang-design

[style-guide] Breaking of complex pattern match expressions

Open
#725 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
F#
Stars
557
Forks
149
Avg merge
4d 13h
Merged PRs (30d)
2

Description

This is from the F# compiler:

```f#
match ty with
| SynType.App (typeName, _, typeArgs, _, _, _, _)
| SynType.LongIdentApp (typeName, _, _, typeArgs, _, _, _) -> typeName :: typeArgs |> List.tryPick (traverseSynType path)
| SynType.Fun (argType = ty1; returnType = ty2) -> [ ty1; ty2 ] |> List.tryPick (traverseSynType path)
| SynType.MeasurePower (ty, _, _)
| SynType.HashConstraint (ty, _)
| SynType.WithGlobalConstraints (ty, _, _)
| SynType.Array (_, ty, _) -> traverseSynType path ty
| SynType.StaticConstantNamed (ty1, ty2, _)
| SynType.Or (ty1, ty2, _, _) -> [ ty1; ty2 ] |> List.tryPick (traverseSynType path)
| SynType.Tuple (path = segments) -> getTypeFromTuplePath segments |> List.tryPick (traverseSynType path)
| SynType.StaticConstantExpr (expr, _) -> traverseSynExpr [] expr
| SynType.Paren (innerType = t)
| SynType.SignatureParameter (usedType = t) -> traverseSynType path t
| SynType.Anon _
| SynType.AnonRecd _
| SynType.LongIdent _
| SynType.Var _
| SynType.StaticConstant _ -> None
```

Apart from a few spaces, this is also how Fantomas formats it. I'm ignoring those spaces in the following.

The expression is complicated enough that I'm having a hard time separating the clauses from the bodies. This is complicated by the fact that some clauses are joined (they use the same `->`), and others are not. The code is not easily readable.

I don't have a definite general rule that can be applied, but I certainly find this more readable:

```f#
match ty with
| SynType.App (typeName, _, typeArgs, _, _, _, _)
| SynType.LongIdentApp (typeName, _, _, typeArgs, _, _, _) ->
typeName :: typeArgs |> List.tryPick (traverseSynType path)
| SynType.Fun (argType = ty1; returnType = ty2) ->
[ ty1; ty2 ] |> List.tryPick (traverseSynType path)
| SynType.MeasurePower (ty, _, _)
| SynType.HashConstraint (ty, _)
| SynType.WithGlobalConstraints (ty, _, _)
| SynType.Array (_, ty, _) ->
traverseSynType path ty
| SynType.StaticConstantNamed (ty1, ty2, _)
| SynType.Or (ty1, ty2, _, _) ->
[ ty1; ty2 ] |> List.tryPick (traverseSynType path)
| SynType.Tuple (path = segments) ->
getTypeFromTuplePath segments |> List.tryPick (traverseSynType path)
| SynType.StaticConstantExpr (expr, _) ->
traverseSynExpr [] expr
| SynType.Paren (innerType = t)
| SynType.SignatureParameter (usedType = t) ->
traverseSynType path t
| SynType.Anon _
| SynType.AnonRecd _
| SynType.LongIdent _
| SynType.Var _
| SynType.StaticConstant _ ->
None
```

Here, I have placed all bodies on separate lines. This is of course not a rule that can be applied generally (it would look worse than single-line placement for short/simple match expressions).

The following also look better, IMHO. Here, bodies that apply to clauses with more than one line are placed on a separate line. Perhaps that can be a general rule?

```f#
match ty with
| SynType.App (typeName, _, typeArgs, _, _, _, _)
| SynType.LongIdentApp (typeName, _, _, typeArgs, _, _, _) ->
typeName :: typeArgs |> List.tryPick (traverseSynType path)
| SynType.Fun (argType = ty1; returnType = ty2) -> [ ty1; ty2 ] |> List.tryPick (traverseSynType path)
| SynType.MeasurePower (ty, _, _)
| SynType.HashConstraint (ty, _)
| SynType.WithGlobalConstraints (ty, _, _)
| SynType.Array (_, ty, _) ->
traverseSynType path ty
| SynType.StaticConstantNamed (ty1, ty2, _)
| SynType.Or (ty1, ty2, _, _) ->
[ ty1; ty2 ] |> List.tryPick (traverseSynType path)
| SynType.Tuple (path = segments) -> getTypeFromTuplePath segments |> List.tryPick (traverseSynType path)
| SynType.StaticConstantExpr (expr, _) -> traverseSynExpr [] expr
| SynType.Paren (innerType = t)
| SynType.SignatureParameter (usedType = t) ->
traverseSynType path t
| SynType.Anon _
| SynType.AnonRecd _
| SynType.LongIdent _
| SynType.Var _
| SynType.StaticConstant _ ->
None
```

To be clear, in this particular case, I still find it less readable than the "always break" alternative. For example, I have to look thorouthly to see that `| SynType.StaticConstantExpr (expr, _) -> traverseSynExpr [] expr` is a clause and a body, and not just a long clause.

I'm not heavily invested in this, but thought I'd raise the issue when I noticed it.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.