Formatter overly opinionated, enforces intent-clobbering F# style

[bryanedds]

Myself and a few others at Jet are adopting an F# coding standard. Unfortunately, the formatter actively works against this standard, even though the standard was specifically designed to be enforceable by just such a tool. Not only are the default settings of the format opinionated such that it fights against this style, there are no options in many cases to align its behavior with said style (or any other than the default!)

This should really be fixed so that we can actually use the formatter like we could in C# (where the default C# formatter was actually nearly perfect to match most professional C# standards, rather than the one the author of the tool seemed to personally prefer).

For more concrete info, here are the portions of the code standard that relate to F# formatting -

A) Correctness

14) Prefix option bindings as well as bindings to potentially null values with 'opt'.

15) Prefix choice bindings with 'chc'.

16) Prefix functions that return an option or potential null with 'try', or another such indicator like 'getOpt'.

17) Prefix functions that return a choice with 'attempt', or another such indicator like 'getChc'.

B) Consistency

3) Use the standard F# naming conventions by -

using UpperCamelCasing for Namespaces, Modules, Types, Fields, Constants, Properties, and InstanceMembers (but as said above, avoid authoring Properties and InstanceMembers in the first place).
using lowerCamelCasing for variables, functions, staticMembers, parameters, and 'typeParameters.

6) Place 'open' statements at the top of each file, right below the current namespace declaration (if any).

9) Prefer stepped indentation as it refactors better, keeps lines shorter, and keeps formatting normal and enforcible via automation. For example, write this -

let result =
    ingest
        apple
        banana
        caribou

rather than this -

let result = ingest apple
                    banana
                    caribou

10) F# is based on ML, which is more based on Lisp than C, so use Lisp-style bracing instead of C-style. For example, write this -

let ys =
    [f x
     g x
     h x]

rather than this -

let ys =
    [
        f x
        g x
        h x
    ]

and this -

type T =
    { M : int
      N : single }

rather than this -

type T =
    {
      M : int
      N : single
    }

11) Tab out discriminated union case definitions to keep them lined up with their members. For example, write this -

type T =
    | A of int
    | B of single
    static member makeA i = A (i * 2)
    static member makeB s = B (s * 2.0f)

rather than this -

type T =
| A of int
| B of single
    static member makeA i = A (i * 2)
    static member makeB s = B (s * 2.0f)

12) Handle the intentional case first when matching / if'ing -

let fn optValue =
    match optValue with
    | Some value -> // do what we actually intended to do in this function
    | None -> // handle the missing case

13) Surround tuples with parens to keep evaluation ordering and intent clear. For example, write this -

let (a, b) = (b, a)

rather than this -

let a, b = b, a

14) Conceptually, () is unit, so please treat it as such. For example, write 'fn ()' rather than 'fn()'.

15) Conceptually, (a, b, c) is a tuple, so please treat it as such. For example, write 'fn (a, b, c)' rather than 'fn(a, b, c)'.

D) And Generally...

1) Use an automated code formatter to enforce this coding standard as much as possible. Note that this is a work-in-progress due to a lack of existing tooling for F#.

2) When appending to this standard, prefer the style that is most enforcible by a reasonably intelligent automated code formatter.

EDIT: The full code standard is posted here - https://github.com/bryanedds/FPWorks/blob/master/STANDARD.md

[vasily-kirichenko]

Prefix option bindings as well as bindings to potentially null values with 'opt'. Prefix choice bindings with 'chc'.

Hungarian notation?

[bryanedds]

Probably the most infuriating thing about the code formatter is how it removes new lines (it used to arbitrarily insert them based on control constructs used, but thankfully now that seems gone). For example, it takes nice, coherently commented code like this -

/// Apply a single refinement to an asset.
let refineAssetOnce intermediateFileSubpath intermediateDirectory refinementDirectory refinement =

    // build the intermediate file path
    let intermediateFileExtension = Path.GetExtension intermediateFileSubpath
    let intermediateFilePath = Path.Combine (intermediateDirectory, intermediateFileSubpath)

    // build the refinement file path
    let refinementFileExtension = getAssetExtension2 intermediateFileExtension refinement
    let refinementFileSubpath = Path.ChangeExtension (intermediateFileSubpath, refinementFileExtension)
    let refinementFilePath = Path.Combine (refinementDirectory, refinementFileSubpath)

    // refine the asset
    ignore <| Directory.CreateDirectory ^ Path.GetDirectoryName refinementFilePath
    match refinement with
    | PsdToPng ->
        use image = new MagickImage (intermediateFilePath)
        writeMagickImageAsPng refinementFilePath image
    | OldSchool ->
        use image = new MagickImage (intermediateFilePath)
        image.Scale (Percentage 400)
        writeMagickImageAsPng refinementFilePath image

    // return the latest refinement localities
    (refinementFileSubpath, refinementDirectory)

and turns it into this garbled mess -

/// Apply a single refinement to an asset.
let refineAssetOnce intermediateFileSubpath intermediateDirectory refinementDirectory refinement = 
    // build the intermediate file path
    let intermediateFileExtension = Path.GetExtension intermediateFileSubpath
    let intermediateFilePath = Path.Combine(intermediateDirectory, intermediateFileSubpath)
    // build the refinement file path
    let refinementFileExtension = getAssetExtension2 intermediateFileExtension refinement
    let refinementFileSubpath = Path.ChangeExtension(intermediateFileSubpath, refinementFileExtension)
    let refinementFilePath = Path.Combine(refinementDirectory, refinementFileSubpath)
    // refine the asset
    ignore <| Directory.CreateDirectory ^ Path.GetDirectoryName refinementFilePath
    match refinement with
    | PsdToPng -> 
        use image = new MagickImage(intermediateFilePath)
        writeMagickImageAsPng refinementFilePath image
    | OldSchool -> 
        use image = new MagickImage(intermediateFilePath)
        image.Scale(Percentage 400)
        writeMagickImageAsPng refinementFilePath image
    (// return the latest refinement localities
     refinementFileSubpath, refinementDirectory)

I use blocks of code with a comment at the top to indicate what that code is doing. The formatter's opinionated-ness destroys this, and gives me no option to tell it to lay off my intended new lines.

[bryanedds]

Yes, people should use this notation on bindings that can potentially be null so people will remember to check for it, and use it on options so that when you bind to the actual value of the opt it does not have to shadow the name.

At any rate, the code formatter in this tool is unusable to me and my team for these reasons. It's just way too opinionated.

[tpetricek]

I agree with pretty much all of the code formatting guidelines that you mentioned in the initial post, especially the spacing. (I do not always use opt and I have never used chc, but aside from that, I like the rules :-)). That said, I know a number of people who have different opinions on this, so having this available as options that can be configured in VFPT would be ideal.

I never really needed to enforce this with a tool, but I can see how that would be useful. So I don't have much experience with how the VFPT formatter works (I use it just on very ugly code :)).

Since VFPT is an open-source project, it would be really nice to see a contribution from you or other folks at Jet that would make it possible to enforce these guidelines. AFAIK the formatter is based on @dungpa's Fantomas project, so perhaps you can ask @dungpa for pointers where to start and what would be needed to implement this?

[vasily-kirichenko]

I don't like most of the proposed rules, but it's OK, I've never met two people who has identical code formatting habits.

What we really need is a number of customization points in Fantomas and a nice preview UI in VFPT, like this:

It's clear that this is a lot of work. In this project features are usually done by a programmer who really needs it in his daily job. Long story short, contribution from Jet is very welcome :)

[bryanedds]

Okay, I will see if resources can be allocated.

[bryanedds]

As such, can someone please assign this issue to me?

[vasily-kirichenko]

Only maintainers can be assigned to an issue.

[bryanedds]

Can you assign me maintainership?

[vasily-kirichenko]

Why? I don't know who are you, sorry.

[tpetricek]

In practice, I think the only purpose of "assigning" things in GitHub is to keep track of things that someone is actively working on to make sure that no one else starts doing the same thing. So, I think it's perfectly sufficient to have the discussion in the comments here. If someone wants to work on this, they should coordinate with @bryanedds first. I'd consider this assigned :-)

[7sharp9]

It seems to be that the overly opinionated formatting will be replaced with another overly opinionated formatting :-)

Not even msdn has consistent formatting for F# there are at least three different indent formats used over DU's types and patten matching. Personally I don't like type prefixes at all, looks like 80s c code...

So, choice in the UI or settings is always a good thing, in XS we have the same set of options as above.

[dsyme]

FWIW I agree with the following:

9) Prefer stepped indentation 10) Use Lisp-style bracing instead of C-style. 11) Tab out discriminated union case definitions to keep them lined up with their members.

The formatter seems to apply these rules with the default settings (or at least with the ones I have which I think are the defaults). I prefer

let ys =
    [ f x
      g x
      h x ]

to

let ys =
    [f x
     g x
     h x]

which the defaults also apply.

Agreed that the formatter should treat existing blank lines with more respect. I think everyone likes that idea.

[bryanedds]

Dave brings up a good thing to look out for - there's little use in exchanging one overly-opinionated formatter with another. The last thing we want is for the formatter to be used as a trojan horse to get someone's pet style into the wider community.

Don, yes, I'm personally quite ambivalent on space after '[', but I had to pick one and go with it.

All, I think the bottom line is that we just need to put some resources into making the formatter much more customizable, and try to find a sufficiently neutral set of settings to deploy with by default. That is, definitely not the one that is currently shipped with.

Again, I'll talk to a couple people about getting resources soon.

Cheers!

[7sharp9]

I've never known why pattern matching is not indented but DU's types are?

The Importance here is more polishing on tooling which can only be a good thing. I wanted the display of formatting for XS too as this is how C# is presented. Formatting can be quirky in XS due to mono style formatting being different to the default VS style formatting, but I guess we can standardise mirror less for F# over both platforms.

On 7 Aug 2015, at 19:29, Bryan Edds notifications@github.com wrote:

Dave brings up a good thing to look out for - there's little use in exchanging one overly-opinionated formatter with another. The last thing we want is for the formatter to be used as a trojan horse to get someone's pet style into the wider community.

Don, yes, I'm personally quite ambivalent on space after '[', but I had to pick one and go with it.

All, I think the bottom line is that we just need to put some resources into making the formatter much more customizable, and try to find a sufficiently neutral set of settings to deploy with by default. That is, definitely not the one that is currently shipped with.

Again, I'll talk to a couple people about getting resources soon.

Cheers!

— Reply to this email directly or view it on GitHub.

[bryanedds]

As I presume you know, the reason pattern matching isn't indented is so our code doesn't tab out to the next town :)

Also, as you know, DUs are a little different as they need to use tabbing to delimit member function definitions as well as their members. While I think it's syntactically valid to have untabbed DU members along with tabbed member functions, I suppose it is a little more visually honest* to just tab it all the same way.

• Where 'visually honest' is defined roughly here - https://vimeo.com/97329157

[vasily-kirichenko]

and try to find a sufficiently neutral set of settings to deploy with by default. That is, definitely not the one that is currently shipped with.

I disagree. Current default formatting is good. That said, I forget when I used the feature last time :)

I also don't think formatting should rename anything. 1. It's formatting after all 2. Renaming many symbols would take forever 3. It'd better be served by the linter.

[bryanedds]

vasily - I never meant to imply that the formatter should rename anything. I would in fact be livid if a code formatter did such a thing! I was just trying to show the stylistic rules some of us are adopting which are related to the more general subject of coding style in F#.

Sorry if mislead the conversation - that was not at all my intent.

[vasily-kirichenko]

Ah, I see :) Sorry.

[7sharp9]

I've never indented DU's, having them line up like matches make more sense to me. Shrug.

On 7 Aug 2015, at 20:34, Bryan Edds notifications@github.com wrote:

vasily - I never said the formatter should rename anything. I was just showing you the stylistic rules some of us are adopting which might be related to the subject at hand.

Sorry if mislead the conversation - that was not at all my intent.

— Reply to this email directly or view it on GitHub.

[7sharp9]

Bryan, if there are any Jet people who want to improve F# on the XS side please send them over to me! :-)

On 7 Aug 2015, at 20:34, Bryan Edds notifications@github.com wrote:

vasily - I never said the formatter should rename anything. I was just showing you the stylistic rules some of us are adopting which might be related to the subject at hand.

Sorry if mislead the conversation - that was not at all my intent.

— Reply to this email directly or view it on GitHub.

[realsnick]

@7sharp9 email me at ido@jet.com

[bryanedds]

BTW - I have yet to follow up about getting said resources; I'm currently utilizing all of my political capital to get an existing project up and running. Should be able to propagate said request next week.

[ImaginaryDevelopment]

the link to your coding standards is dead, can you post a fresh one @bryanedds

[bryanedds]

Done :)

[vasily-kirichenko]

As @bryanedds no longer works for Jet, I believe there is no chance to move this feature forward.

[bigjonroberts]

Now that the cut-over to Roslyn in VS2017 is complete, could this be revisited with a plugin-based approach?