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

Experience report upgrading to v4.6.0-alpha-006 #1961

Closed
pbiggar opened this issue Nov 8, 2021 · 4 comments
Closed

Experience report upgrading to v4.6.0-alpha-006 #1961

pbiggar opened this issue Nov 8, 2021 · 4 comments

Comments

@pbiggar
Copy link
Contributor

pbiggar commented Nov 8, 2021

Hey, this is an experience report of upgrading to Fantomas v4.6.0-alpha-006+c90333468261671f2f0e8e065009160b74a41662 from version 4.5.4.

Changes I didn't like :)

Brace on new line

   testTask $"{api} API returns same" {
-    return! postApiTestCases api body deserialize canonicalizeBody }
+    return! postApiTestCases api body deserialize canonicalizeBody
+  }
         test $"{name}[{i}]: ({input}) -> {expected}" {
-          Expect.equal (fn input) expected "" })
+          Expect.equal (fn input) expected ""
+        })

I think this changed for CEs, but it's not consistent with what happens for records. Wasn't a huge fan, but could go either way.

Lambdas kept on same line

It seems lambda arguments are changed such that:

  1. the lambda declaration is kept on the same line
  2. the body is indented two from the start of the line

I think I kinda like this, unsure. It gets back two columns in the body, which is nice for sure.

However, in pipelines the body is now indented from the start of the |>:

@@ -29,14 +29,12 @@ let routeToPostgresPattern (route : string) : string =

   // https://www.postgresql.org/docs/9.6/functions-matching.html
   route
-  |> String.collect
-       (function
-       | '%' -> "\\%"
-       | '_' -> "\\_"
-       | other -> string other)
+  |> String.collect (function
+    | '%' -> "\\%"
+    | '_' -> "\\_"
+    | other -> string other)
   |> splitUriPath
-  |> Array.map
-       (fun segment -> if String.startsWith ":" segment then "%" else segment)
+  |> Array.map (fun segment -> if String.startsWith ":" segment then "%" else segment)
   |> String.concat "/"
   |> (+) "/"
@@ -192,11 +189,10 @@ let loadEventForTrace
          AND trace_id = @traceID
        LIMIT 1"
   |> Sql.parameters [ "canvasID", Sql.uuid canvasID; "traceID", Sql.uuid traceID ]
-  |> Sql.executeRowOptionAsync
-       (fun read ->
-         (read.string "path",
-          read.dateTime "timestamp",
-          read.string "value" |> LibExecution.DvalRepr.ofInternalRoundtrippableV0))
+  |> Sql.executeRowOptionAsync (fun read ->
+    (read.string "path",
+     read.dateTime "timestamp",
+     read.string "value" |> LibExecution.DvalRepr.ofInternalRoundtrippableV0))

I find it harder to read the pipelines now. Also, it is inconsistent, as it's indented differently in the pipeline vs no pipeline:

let x y =
  do
    callFunc (fun a b c ->
      let x = b + c
      let z = a + y
      x + z)

  x
  |> callFunc (fun a b c ->
    let x = b + c
    let z = a + y
    x + z)

No settings for new changes

The new changes led to quite a big diff. It would be better if I could disable the changes using settings: this would allow me have a small diff that I could manually verify (this is important as I'm still of the impression that fantomas can lose code/comments sometimes).

How to use

The reason I tried v4.6 was that I wanted to decouple ionide from fantomas so I could control the version of fantomas used for all my project's users. However, I could figure out how to use it in ionide 5.8.1. I don't remember where I got the impression that that's how this works (some issue somewhere I think) - apologies if this was an error in my understanding.

@nojaf
Copy link
Contributor

nojaf commented Nov 8, 2021

Hello Paul,

Changes I didn't like :)

I'm afraid you are preaching to the choir here. As mentioned in your last experience report Fantomas follows the MS style guide. All stylistic changes were introduced here because of prior changes in the guide.
You can engage in conversation in this guide as explained in this video.

The change about lambdas was discussed in dotnet/docs#25207 and I'm well aware this will have caused a large diff because of this. It was not treaded lightly, though it is unlikely that this will not happen again in future versions.

No settings for new changes

We don't have the bandwidth to support every stylistic preference of each end-user. I have to deal with these conversations on a weekly basis and that is why we settled on two style guides. (//cc @dsyme).
Every setting really has a high maintenance cost and give the current stability of the project, less is more.

How to use

4.6 is indeed the first version that will decouple Fantomas from Ionide as proposed in #1844.
You probably at some point had a similar experience as shown here.

Thanks for understanding,

Florian

@pbiggar
Copy link
Contributor Author

pbiggar commented Nov 8, 2021

Completely understood, just wanted to provide the feedback. Looks like there's nothing actionable here so will close.

@pbiggar pbiggar closed this as completed Nov 8, 2021
@nojaf
Copy link
Contributor

nojaf commented Nov 8, 2021

Thanks Paul

@dsyme
Copy link
Contributor

dsyme commented Nov 8, 2021

Thanks for the feedback Paul

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

No branches or pull requests

3 participants