Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[style-guide] Breaking of complex pattern match expressions #725

Open
cmeeren opened this issue Dec 26, 2022 · 1 comment
Open

[style-guide] Breaking of complex pattern match expressions #725

cmeeren opened this issue Dec 26, 2022 · 1 comment

Comments

@cmeeren
Copy link

cmeeren commented Dec 26, 2022

This is from the F# compiler:

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:

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?

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.

@nojaf
Copy link
Contributor

nojaf commented Dec 28, 2022

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

Please respect the rules regarding style guide changes.

Adjustments to the style guide should generally only be made with consideration about their implementability in Fantomas, and if an adjustment is approved you should be prepared to contribute a matching pull request to Fantomas.

If there is no interest to commit, you should not propose any changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants