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

Option for consistent indentation style of all pattern matching branches #2885

Closed
4 of 6 tasks
iblazhko opened this issue May 26, 2023 · 1 comment
Closed
4 of 6 tasks

Comments

@iblazhko
Copy link

iblazhko commented May 26, 2023

I propose we add an option for consistent indentation style of all pattern matching branches.

The existing way of Fantomas deals with this problem is that it takes into account line length of an individual branch expression.

As a result of that, some branches may be split into multiple lines with indentation, while other branches are in single line.

I believe it would improve readability (at least in some cases) if all pattern matching branches were indented consistently.

Pros and Cons

The advantages of making this adjustment to Fantomas are:

  • long match expressions are more readable

The disadvantages of making this adjustment to Fantomas are:

  • short branches will be indented although they would have fit line width without indentation, so some may consider it unnecessary

Examples

Consider this initial source code:

let fromDomain (evt: InventoryEvent) =
    match evt with
    | InventoryCreated x -> 
        x |> InventoryCreatedMapper.fromDomain :> CqrsEventDto
    | ItemInStock x ->
        x |> ItemInStockMapper.fromDomain :> CqrsEventDto
    | ItemWentOutOfStock x ->
        x |> ItemWentOutOfStockMapper.fromDomain :> CqrsEventDto

let toDomain (dto: CqrsEventDto) =
    match dto with
    | :? InventoryCreatedEvent as x ->
        x
        |> InventoryCreatedMapper.toDomain
        |> Result.map InventoryEvent.InventoryCreated
    | :? ItemInStockEvent as x ->
        x
        |> ItemInStockMapper.toDomain
        |> Result.map InventoryEvent.ItemInStock
    | :? ItemWentOutOfStockEvent as x ->
        x
        |> ItemWentOutOfStockMapper.toDomain
        |> Result.map InventoryEvent.ItemWentOutOfStock
    | x ->
        raise (EventDtoMappingException $"Unknown event DTO type: {x.GetType().FullName}")

Fantomas 6.0 with default settings will produce following formatted code:

let fromDomain (evt: InventoryEvent) =
    match evt with
    | InventoryCreated x -> x |> InventoryCreatedMapper.fromDomain :> CqrsEventDto
    | ItemInStock x -> x |> ItemInStockMapper.fromDomain :> CqrsEventDto
    | ItemWentOutOfStock x -> x |> ItemWentOutOfStockMapper.fromDomain :> CqrsEventDto

let toDomain (dto: CqrsEventDto) =
    match dto with
    | :? InventoryCreatedEvent as x ->
        x
        |> InventoryCreatedMapper.toDomain
        |> Result.map InventoryEvent.InventoryCreated
    | :? ItemInStockEvent as x -> x |> ItemInStockMapper.toDomain |> Result.map InventoryEvent.ItemInStock
    | :? ItemWentOutOfStockEvent as x ->
        x
        |> ItemWentOutOfStockMapper.toDomain
        |> Result.map InventoryEvent.ItemWentOutOfStock
    | x -> raise (EventDtoMappingException $"Unknown event DTO type: {x.GetType().FullName}")

Note that the branch :? InventoryCreatedEvent is indented while the branch :? ItemInStockEvent is not indented.

I don't mind the changes in the 1st match in the formatted output, but to my eye 2nd match looks misaligned, it becomes even more apparent when more branches of mixed length are added to that match.

Extra information

Estimated cost (XS, S, M, L, XL, XXL): M? (No idea TBH)

Related suggestions: N/A

Affidavit (please submit!)

Please tick this by placing a cross in the box:

Please tick all that apply:

@nojaf
Copy link
Contributor

nojaf commented May 26, 2023

Hello, thank you for your interest in this project.
This request has been raised before and is being discussed at fsharp/fslang-design#650.
This should be settled by the style guide.
See what-are-we-not-looking-for and Default-style-guide

@nojaf nojaf closed this as not planned Won't fix, can't repro, duplicate, stale May 26, 2023
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