Is auto-formatting code useful or in the way?

[ploeh]

It took a few Git commits before it dawned on me that, as the contribution guideline explains, code is auto-formatted on each commit.

Mostly, I don't mind.

Some of the rules, however, strike me as less than useful. One example is that the formatter seems to insist on adding a newline before or after each list/array expression. I can't quite figure out what the rule is, and scanning the Fantomas documentation hasn't much helped. Perhaps I'm just not looking closely enough, or in the wrong place...

In any case, I was writing a unit test like this:

Tests.test "getTotals can return non-empty Totals" {
    let td: Informedica.GenForm.Lib.Types.Data.TotalsData[] =
        [|
            {
                Name = "eiwit"
                MinAge = None
                MaxAge = None
                MinWeight = None
                MaxWeight = None
                Unit = Some Units.Mass.gram
                Adj = None
                TimeUnit = Some Units.Time.day
                MinPerTime = None
                MaxPerTime = None
                MinPerTimeAdj = None
                MaxPerTimeAdj = None
            }
        |]
    let age = None
    let weight = 10N |> ValueUnit.singleWithUnit Units.Weight.kiloGram |> Some
    let dtos = [| Order.Dto.once "1" "voeding" "oraal" [ ("dummy", "vloeistof", [ "eiwit" ]) ] |]

    let actual = Informedica.GenOrder.Lib.Totals.getTotals td age weight dtos

    emptyTotals <>! actual
}

Although top-heavy, it follows my simple heuristic for formatting tests according to the AAA pattern: There are three distinct sections of the test, each demarcated by a blank line.

Once committed, however, this test becomes:

Tests.test "getTotals can return non-empty Totals" {
    let td: Informedica.GenForm.Lib.Types.Data.TotalsData[] =
        [|
            {
                Name = "eiwit"
                MinAge = None
                MaxAge = None
                MinWeight = None
                MaxWeight = None
                Unit = Some Units.Mass.gram
                Adj = None
                TimeUnit = Some Units.Time.day
                MinPerTime = None
                MaxPerTime = None
                MinPerTimeAdj = None
                MaxPerTimeAdj = None
            }
        |]

    let age = None
    let weight = 10N |> ValueUnit.singleWithUnit Units.Weight.kiloGram |> Some

    let dtos =
        [|
            Order.Dto.once "1" "voeding" "oraal" [ ("dummy", "vloeistof", [ "eiwit" ]) ]
        |]

    let actual = Informedica.GenOrder.Lib.Totals.getTotals td age weight dtos

    emptyTotals <>! actual
}

The test now contains five 'sections', and at a glance it's not clear what belongs where.

One may, of course, follow the heuristic and consequently add comments to delineate the three phases:

Tests.test "getTotals can return non-empty Totals" {
    // Arrange
    let td: Informedica.GenForm.Lib.Types.Data.TotalsData[] =
        [|
            {
                Name = "eiwit"
                MinAge = None
                MaxAge = None
                MinWeight = None
                MaxWeight = None
                Unit = Some Units.Mass.gram
                Adj = None
                TimeUnit = Some Units.Time.day
                MinPerTime = None
                MaxPerTime = None
                MinPerTimeAdj = None
                MaxPerTimeAdj = None
            }
        |]

    let age = None
    let weight = 10N |> ValueUnit.singleWithUnit Units.Weight.kiloGram |> Some

    let dtos =
        [|
            Order.Dto.once "1" "voeding" "oraal" [ ("dummy", "vloeistof", [ "eiwit" ]) ]
        |]

    // Act
    let actual = Informedica.GenOrder.Lib.Totals.getTotals td age weight dtos

    // Assert
    emptyTotals <>! actual
}

This just seems unnecessarily verbose for no apparent reason.

To be clear, I can appreciate consistency in coding style, but there's an argument to be made for allowing a degree of freedom. Here, for example, I find that the best code layout is contextual. What may work well in a function may not be optimal in a test.

Does using an auto-formatter solve a real problem?

[halcwb]

@ploeh

Could you try in the editor.config:

fsharp_blank_lines_around_nested_multiline_expressions = false
fsharp_max_array_or_list_width = 100
fsharp_max_value_binding_width = 100

That should solve your problem.

[ploeh]

Assuming you meant .editorconfig, then yes, that 'solves' the problem, but does it address the underlying question?

In order to allow this kind of formatting, I have to check in .editorconfig, which would then impact all existing code. Would that be appropriate?

We could, of course do that, but my point is rather that in my experience, the best way to format a particular block of code is context-dependent. I don't believe that one universal formatting convention is appropriate, but that's just my opinion.

I can live with the situation as it is, but wanted to bring up the question.

[halcwb]

As to the underlying question:

First of all, I completely agree with you that you should be able to give formatting "semantic meaning". If the autoformat process interferes with this we should try to fix this.

As to why in the first place, this is about readability as well. When you have a consistent formatting scheme reading the code should become easier as it will be more predictable from the pattern of the code what the code is. But also, it makes editing the code easier. For example, if some one was going to use cramped style for list expressions and you want to copy paste that code block with a different indentation, that breaks syntax. In that past that was really annoying.

Finally, the .editorconfig is in the repository, so, it should set all collaborators on the same page with regard to formatting.

[ploeh]

Finally, the .editorconfig is in the repository, so, it should set all collaborators on the same page with regard to formatting.

If we change .editorconfig it may solve the problem reported here, related to tests, but then are we sure it doesn't mess up formatting somewhere else?

[halcwb]

@7sharp9 : I have created a .editorconfig to see what happens when applied to GenPRES.

Could you give this a try?

Also: the old lf vs CRLF issue. Not sure if we handle this correctly. I always forget how this exactly works. As long as it doesn't pollute the git commits.

[7sharp9]

@7sharp9 : I have created a .editorconfig to see what happens when applied to GenPRES.

Could you give this a try?

Also: the old lf vs CRLF issue. Not sure if we handle this correctly. I always forget how this exactly works. As long as it doesn't pollute the git commits.

So use your gist to replace the existing one?

[halcwb]

@7sharp9 : I have created a .editorconfig to see what happens when applied to GenPRES.
Could you give this a try?
Also: the old lf vs CRLF issue. Not sure if we handle this correctly. I always forget how this exactly works. As long as it doesn't pollute the git commits.

So use your gist to replace the existing one?

Yeah, I deliberately put it in an gist so you can just try it in an experimental fork/clone of GenPRES on your own machine.