fsprojects / FSharp.AWS.DynamoDB

F# wrapper API for AWS DynamoDB
MIT License
58 stars 18 forks source link

Apply standard formatting across the board #73

Closed samritchie closed 7 months ago

samritchie commented 7 months ago

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 9e080622c5c0fccd808b934fdd140101c75c2c04

bartelink commented 7 months ago

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!

bartelink commented 7 months ago

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. https://github.com/jet/equinox/pull/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 commented 7 months ago

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

samritchie commented 7 months ago

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

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...

bartelink commented 7 months ago

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 commented 7 months ago

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

samritchie commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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