Apply standard formatting across the board #73

samritchie · 2024-02-02T07:10:04Z

This applies pretty much just the default fantomas rules as a baseline & to get the monster diff out of the way. I’m not particularly religious about style so open to change if needed.

Also addressed previous test feedback in 9e08062

bartelink · 2024-02-02T08:27:40Z

Not a bad idea in general. Have been meaning to get into it myself as it has reached a stage...
But there's def scope to tweak the default ruleset - @abelbraaksma has a lovingly tweaked ruleset in
https://github.com/fsprojects/FSharp.Control.TaskSeq/blob/main/src/.editorconfig - I worked in that repo for a while and it felt just like it was being 'correctly' formatted - then he goes and reveals its fantomas with good rules.
Unfortunately to deeply understand the ruleset is not a 2 minute job, but I anticipate starting from it as a baseline

Right, now I'll actually go look about what annoys me about the default ruleset, hoping to be pleasantly surprised!

build.fsx

src/FSharp.AWS.DynamoDB/Expression/ConditionalExpr.fs

bartelink · 2024-02-02T08:53:06Z

src/FSharp.AWS.DynamoDB/Expression/ConditionalExpr.fs

+        | False
+        | True -> invalidOp "internal error: invalid query representation"
+        | Not q ->
+            ! "( NOT "


this sort of stuff is just bad news - maybe if there was a prettier ignore annotation you could att at function level?

Pretty sure you can only ignore at file level

maybe thats the thing to do as the bulk of files do seem mostly fine overall

seems cured, will delete this thread after

wait, not sure why I thought it was cured, leaving comment here

src/FSharp.AWS.DynamoDB/Expression/ConditionalExpr.fs

samritchie · 2024-02-02T09:03:25Z

src/FSharp.AWS.DynamoDB/Expression/ConditionalExpr.fs

@@ -515,11 +585,10 @@ type ConditionalExpression with
        let values = aw.Values |> Seq.map (fun kv -> kv.Key, kv.Value.Print()) |> Seq.toList
        expr, names, values

-    static member Extract (recordInfo : RecordTableInfo) (expr : Expr) =
-        extractQueryExpr recordInfo expr
+    static member Extract (recordInfo: RecordTableInfo) (expr: Expr) = extractQueryExpr recordInfo expr


I think this is all based on line length - it’s not necessarily going to be consistent

bartelink · 2024-02-02T09:08:16Z

OK, ultimately I know fantomas is the right thing to do
But it's got some really painful defaults, and F# the language is not as amenable to blanket rules as some noiser languages

The result is that it works really badly with handf ormatted consistent code like equinox, e.g. jet/equinox#345 (see my mainline comment there on my stance)

What to do here? Main thing is to measure twice, cut once.

The main risk here is that a hand formatted parser with necessarily large blocks of logic which is non trivial to understand is really not helped by being stretched to twice the line count - there were enough of those in my quick traverse to nor make me want to do it if it was my project.

While the formatter demonstrates that Eirik was not 100% consistent, ultimately his code in 2014 was setting standards for legible layout without any fantomas in the picture. Regularising it is going to lose some value. But there is a win too if it aligns with sister projects that people are likely to have read and/or eventually will read in conjunction with it.

My hunch is that taking the TaskSeq ruleset and living with that might be a better answer as it didnt offend me too much despite being picky and working across plenty projects.

But I won't fight on using the defaults, as plenty projects do that and its not like the fantomas folk dont have internal style debates.

bartelink · 2024-02-02T09:11:50Z

also nojaf (was going to at him, but people have enough to do) and I have yet to apply it to Argu, which will likely have similar issues
Can't see it working on TypeShape either for similar reasons that the parser logic gets mangled.
would nearly at Eirik here but he's done enough for us to not deserve the harassment of being dragged into a style debate

src/FSharp.AWS.DynamoDB/Expression/ExprCommon.fs

samritchie · 2024-02-02T11:15:38Z

Okay, I’ve been through the TaskSeq config and removed the obsolete settings etc.

Still no luck with the pattern

if condition then doSomethingOnOneLine
else dont

it seems to be either all on one line, or split across 4. elifs seem to allow the case body inline though

With the space-before-DU the only setting is fsharp_space_before_uppercase_invocation which is also going to catch MethodCall ()

Agree in general re formatters - I avoided them for a long time but they are quite liberating once you get used to them. Sure it doesn’t always result in code the way I would have done it, but I didn’t have to do it...

src/FSharp.AWS.DynamoDB/Expression/ConditionalExpr.fs

bartelink · 2024-02-02T11:20:26Z

src/FSharp.AWS.DynamoDB/Expression/ConditionalExpr.fs

+
+        let names =
+            aw.Names
+            |> Seq.map (fun kv -> kv.Key, kv.Value)


maaaan (though the answer is to rewrite it as [ for kv in aw.Names -> kv.Key, kv.Value])

bartelink · 2024-02-02T11:22:22Z

src/FSharp.AWS.DynamoDB/Expression/ProjectionExpr.fs

-                sprintf "Document path '%s' not found." id.Name |> KeyNotFoundException |> raise
+                sprintf "Document path '%s' not found." id.Name
+                |> KeyNotFoundException
+                |> raise


argh! I guess the rule is >=2 pipes means multi line
I'd prob write raise (KeyNotFoundException($"...")) if it did this to me
but fine

bartelink · 2024-02-02T11:24:39Z

Looking better but seems cure is worse than disease if spaces before DU ctor args means space before method args and all other ctor args
If we get it down to one or two nigglies and are continuing, then in the end we might at in nojaf to see if there is a workaround/better solution

bartelink · 2024-02-02T11:28:38Z

Okay, I’ve been through the TaskSeq config and removed the obsolete settings etc.

It does seem to be getting decent
one line ifs are prob not the end of the world even if I really like them
but I also like trailing else with a blank line for early exits too

Can prob do stupid things like converting ifs to matches to fight the formatter lol

src/FSharp.AWS.DynamoDB/Expression/ExpressionContainers.fs

bartelink · 2024-02-02T11:38:30Z

src/FSharp.AWS.DynamoDB/Expression/ExpressionContainers.fs

also need to do an F# syntax update - Rider can bulk fix these

src/FSharp.AWS.DynamoDB/Expression/ProjectionExpr.fs

samritchie · 2024-02-04T01:19:21Z

src/FSharp.AWS.DynamoDB/Expression/ProjectionExpr.fs

-                sprintf "Document path '%s' not found." id.Name
-                |> KeyNotFoundException
-                |> raise
+                sprintf "Document path '%s' not found." id.Name |> KeyNotFoundException |> raise


I’m not sold on one-lining these, having the raise at the end of the line is less obvious than having it at the bottom of the chain. I can experiment with winding back the infix expression length a bit but the likelihood is it will catch some & not others.

yes 2 pipes on a line is def the limit
and I prefer the raise to stick out
as mentioned I'd map that to raise (KeyNotFoundException $"...")
but sreading it over 3 lines doesnt really help anyone IMO other than forcing them to think, which is great of course

samritchie · 2024-02-04T01:37:28Z

src/FSharp.AWS.DynamoDB/Expression/ProjectionExpr.fs

+                    values
+                    |> Seq.map (function
+                        | AttributeGet qa -> qa
+                        | _ -> invalidExpr ())


There doesn't appear to be a max expression length for match/function; it always breaks.

maybe that's not the end of the world in this instance - it can work well for short one liner option matches, but I guess its also something that gets taken too far (speaking personally 😊 )

samritchie · 2024-02-11T07:59:14Z

Picking this up again - the main change I’ve made is adding the fsharp_experimental_keep_indent_in_branch for early exit.

I’ve also turned off fsharp_blank_lines_around_nested_multiline_expressions - this is imo the one that annoys the most because you hit length limits then suddenly you have 15 lines instead of 3. Effectively with fsharp_keep_max_number_of_blank_lines set it’s going to allow the author to insert/delete blank lines which are preserved (in most cases). I’ve gone through and manually tidied some blank lines where it looked like it made sense but it’s very subjective.

samritchie · 2024-02-11T08:03:16Z

I think the main part that’s still substantially ugly(er) than previous are those multiple-expression-lines in the query writing code. The option is always there to exclude those and manually format but it’s probably not worth the tradeoff. @bartelink I agree most of the other quirks would be better handled by changing code structure instead. When that will happen I don’t know though...

bartelink · 2024-02-11T12:27:23Z

tests/FSharp.AWS.DynamoDB.Tests/MetricsCollectorTests.fs

@@ -281,11 +271,9 @@ type ManyReadOnlyItemsFixture() =
    let table = base.CreateEmpty<MetricsRecord>()

    let hk = guid ()
-
    do


for these, I'm wondering if the do can go on the same line, or does it separate it if you do (very minor of course)

bartelink · 2024-02-11T12:31:54Z

Looks to have worked very well. On balance its positive for the codebase so I'd call it done.
'We' should try to do basic language updates per Rider suggestions pretty soon as a follow-up.
All that remains is to ponder whether and when I want to do this to github.com/jet

samritchie added 2 commits February 2, 2024 14:37

Updated tests with feedback from @bartelink

9e08062

Reformataggedon

3db21a5

samritchie requested a review from bartelink February 2, 2024 07:10