Consider updating Fantomas & explicitly specifying some config settings

[brianrourkeboll]

Sorry if this has been discussed, planned, or documented elsewhere—please close if so. I know the use of Fantomas in this repo is relatively new, but is there a standard process for updating it? I understand the need to weigh fixing bugs against formatting churn.

Nonetheless, I think we should consider doing these:

1. Update Fantomas, if possible.
2. Explicitly specify more of the Fantomas settings in the repo's .editorconfig, even if we are using the default.

Why update Fantomas

As I ran into a number of times in my last couple of PRs (37142b6, e79cbbe, 83fa32c, etc.), the version of Fantomas currently used in this repo (5.0.3) has inconsistent treatment of discriminated union cases, especially in matches.

Example

@@ -1,9 +1,9 @@
-match Some (Some 123) with
-| Some (Value = Some (Value = 123)) -> ()
-| Some (Value = Some (123)) -> ()
-| Some (Some (Value = 123)) -> ()
-| Some (Value = Some (Value = x)) -> ()
-| Some (Value = x as Some (Value = y)) -> ()
+match Some(Some 123) with
+| Some(Value = Some(Value = 123)) -> ()
+| Some(Value = Some (123)) -> ()
+| Some (Some(Value = 123)) -> ()
+| Some(Value = Some (Value = x)) -> ()
+| Some(Value = x as Some (Value = y)) -> ()
 | Some (Some (123)) -> ()
 | Some (Some 123) -> ()
 | Some (Some (x)) -> ()

The current version of Fantomas (6.2.3) no longer has this problem, and the space is either always kept or always removed.

I see the following variation when I grep this codebase:

With space: A… (…

rg "\b(:?[A-Z][a-z0-9]*)+\s+\(" -tfsharp -o | measure | select count

# Count
# -----
# 26162

Without space: A…(…

rg "\b(:?[A-Z][a-z0-9]*)+\(" -tfsharp -o | measure | select count

#  Count
#  -----
# 111287

Some of that inconsistency is certainly due to the Fantomas bug, but some of the rest is likely expected (chained fluent calls, etc.).

Why specify more settings

Even though I understand the desire to use the default Fantomas settings whenever possible, and for the Fantomas defaults themselves to follow the Microsoft style guide, specifying settings like fsharp_space_before_uppercase_invocation, fsharp_space_before_member, etc., here would remove the need for contributors to consult Fantomas's docs or the style guide separately for clarity.

The Roslyn repo, for example, explicitly specifies many settings even though they use the default value.

[dawedawe]

Thanks for your interest in this.
There's PR #15847 to update Fantomas. I hope that will solve the issues you observed.

But it was decided to park it because of the nullness PR.

[brianrourkeboll]

Ah, I remember seeing that now. Do you have an opinion on my suggestion that we explicitly specify some more settings in this repo's .editorconfig, even if they are the default values?

It would mean that changes like #15847 (comment) would necessarily be more deliberate and explicit and would come as less of a surprise. Right now, especially with the current version lag, there is a series of extra hops required for a contributor to know what to expect: this repo → the Fantomas version currently used by this repo → the defaults for that Fantomas version → the version of the Microsoft style guide as of that Fantomas version.

One argument against specifying more settings explicitly is that there has historically been some amount of churn in the set of options Fantomas exposes, or in the options' names. This repo's .editorconfig already has some obsolete settings, e.g.,

https://github.com/dotnet/fsharp/blob/502aeff9c88e8c28be824b84f1bfefb48851d169/.editorconfig#L13C1-L13C47

I would nonetheless still personally prefer more explicit settings in this repo. Thoughts?

[dawedawe]

I don't have super strong feelings against making the defaults explicit. The plus of maintenance shouldn't be too big as the file rarely changes.
But I really don't think that you can avoid that people get upset about formating choices by doing so. There's just too much personal preference in that space. I'm afraid, no amount of documentation will change that.

So if you still see other benefits from it, sure, give it a go in a PR and see what the maintainers of this repo say.

[brianrourkeboll]

Oh, by "surprised" I didn't mean "upset" (even though I'm also a space-keeper by preference myself 😊)—I just meant that if, in, e.g., #15847, we also explicitly set fsharp_space_before_uppercase_invocation = true in the .editorconfig, it would make the reason for many of the formatting changes in the rest of the PR immediately obvious (setting aside the fact that some are due to fixed bugs).

[brianrourkeboll]

Closing this for now. Things should at least be much more consistent after #15847.