Format most of FSharp.Core #13150

dsyme · 2022-05-18T12:43:21Z

From the compiler code cleanup session last night, this applies auto-formatting to most implementation files in FSharp.Core

There are a lot of changes so we might want to do this in smaller chunks we can review carefully, and possibly make some preliminary adjustments to reduce the diff

+8,971 −5,300 - 120 width

+8,641 −5,286 - 140 width

vzarytovskii · 2022-05-18T13:04:45Z

Oh boy 😆

nojaf · 2022-05-18T13:12:16Z

To have this in smaller chunks, you could negate some ignored files using !.
For example: !src/FSharp.Core/result.fs, this would format result.fs even though **/*.fs was listed above.

src/FSharp.Core/QueryExtensions.fs

vzarytovskii · 2022-05-18T13:23:07Z

src/FSharp.Core/QueryExtensions.fs

+        assert
+            (match res with
+             | NewAnonymousObject _ -> true
+             | _ -> false)


This too, looks a bit weird, I thing oneliner looked more readable.

Matches are never in one line in Fantomas.

@nojaf Can we keep existing one liner if they are short enough to fit on the line (or use some other metric)?

I think we could have a setting for this, to control the max-width to print it as a one-liner.
At first glance, using max_line_length won't work for everyone.
Sounds like a reasonable thing to add to the style guide though.

TBH I look at cases like this as places where the original code needs improvement

It is a tricky case though. People do certainly use one line matches for this purpose

The lack of one line formatting for match union with A -> true | B -> false is a general concern for the style guide, as there is no other way to implement an union.IsA property or check, and that is very common - one of the reasons we don't expose the IsA properties has always been that implementing them is just a trivial match, but now there is a larger penalty for that

So I do wonder whether this shouls be a special case for fantomas, at least until the IsA properties are exposed to F#

src/FSharp.Core/QueryExtensions.fs

dsyme · 2022-05-18T15:16:26Z

Looks like a fantomas bug here:

-type hertz = / second / second
+type hertz = / second / second / second / second

https://github.com/dotnet/fsharp/pull/13150/files#diff-dcae2bcdbe279a9e446b192c415ade297e6a324e9bb1b6de75e1f4b347e9df68R42

src/FSharp.Core/SI.fs

src/FSharp.Core/async.fs

vzarytovskii · 2022-05-18T17:18:49Z

src/FSharp.Core/collections.fs

+    let inline NonStructural<'T when 'T: (static member (<): 'T * 'T -> bool) and 'T: (static member (>):
+        'T * 'T -> bool)> : IComparer<'T> =


Should line breaks instead be aligned with and and only then :?

Yes this looks a little dodgy. @nojaf similar question to above - will constraint formatting affect this? thanks

No, reported fsprojects/fantomas#2267.

vzarytovskii · 2022-05-18T17:19:56Z

src/FSharp.Core/event.fs

+type Event<'Delegate, 'Args when 'Delegate: delegate<'Args, unit> and 'Delegate :> System.Delegate and 'Delegate: not struct>
+    () =


Is it expected to have constructor moved to new line?

I guess so. I mean it's at column 133 and it needs a break :)

@nojaf will the adjusted constraint formatting change this? e.g.

type Event<'Delegate, 'Args when 'Delegate: delegate<'Args, unit> and 'Delegate :> System.Delegate and 'Delegate: not struct> () =

Yeah, I guess that's what I meant, is if it's possible, to have line breaks at the ands.

No, probably not. I created fsprojects/fantomas#2266.

src/FSharp.Core/async.fs

dsyme · 2022-05-18T17:30:41Z

src/FSharp.Core/map.fs

+                .Append("; ")
+                .Append(txt2)
+                .Append("]")
+                .ToString()


https://github.com/dotnet/fsharp/pull/13156/files#diff-0947e2727d6bad8cd0ac4122f5314bb5b04e337393075bc4b5ef143b17fcbd5bR8 may help this (setting fsharp_max_dot_get_expression_width wider)

src/FSharp.Core/map.fs

src/FSharp.Core/QueryExtensions.fs

src/FSharp.Core/map.fs

src/FSharp.Core/QueryExtensions.fs

dsyme · 2022-05-19T11:39:23Z

I have to say, looking through the files I'm finding it hard to spot additional problems - and there are many improvements.

dsyme · 2022-05-19T17:44:58Z

@vzarytovskii @nojaf It feels like the only outstanding issue here is the constrained type parameter declarations.

We could

suppress the use of formatting for those specific files, or
accept them as they are
wait for the fix

I guess (1) ? I'd sort of be OK for (2) but probably better to do (1)

nojaf · 2022-05-19T19:26:18Z

wait for the fix

I might be able to take a look tomorrow.
Maybe it will be something quite similar to the previous fix.

dsyme · 2022-05-19T20:07:28Z

@nojaf thanks! Yes I expect will be very similar

* update fantomas (#13206) * Format most of FSharp.Core (#13150) * modify fantomasignore * fix setting * no single line functions in FSHarp.Core * update fantomas * apply formatting * Format src/Compiler/Driver (#13195) * adjust settings * adjust code * adjust settings * adjust code * fix code before formatting * remove unnecessary yield * manual pre-formatting * preadjust code * preadjust code * preadjust code * preadjust code * adjust settings" * adjust settings" * adjust settings * adjust settings * fix build * adjust settings * adjust code * adjust code * adjust code * update fantomas * apply formatting * apply formatting (fix build) (#13209) * preformat * apply formatting Co-authored-by: Don Syme <[email protected]>

modify fantomasignore

57812cc

vzarytovskii reviewed May 18, 2022

View reviewed changes

src/FSharp.Core/QueryExtensions.fs Outdated Show resolved Hide resolved

vzarytovskii reviewed May 18, 2022

View reviewed changes

src/FSharp.Core/QueryExtensions.fs Show resolved Hide resolved

vzarytovskii reviewed May 18, 2022

View reviewed changes

dsyme mentioned this pull request May 18, 2022

pre-formatting of some files in FSharp.Core #13151

Merged

vzarytovskii reviewed May 18, 2022

View reviewed changes

src/FSharp.Core/QueryExtensions.fs Outdated Show resolved Hide resolved

dsyme added 2 commits May 18, 2022 17:25

merge main

081e294

Merge branch 'main' of https://github.com/dotnet/fsharp into ft15

5d7e177

dsyme force-pushed the ft15 branch from 7ef7f33 to 714ec2b Compare May 18, 2022 16:52

fix setting

36ae0ea

vzarytovskii reviewed May 18, 2022

View reviewed changes

src/FSharp.Core/SI.fs Outdated Show resolved Hide resolved

dsyme force-pushed the ft15 branch from 714ec2b to 5c97267 Compare May 18, 2022 16:57

vzarytovskii reviewed May 18, 2022

View reviewed changes

src/FSharp.Core/async.fs Show resolved Hide resolved

vzarytovskii reviewed May 18, 2022

View reviewed changes

dsyme commented May 18, 2022

View reviewed changes

src/FSharp.Core/async.fs Show resolved Hide resolved

dsyme commented May 18, 2022

View reviewed changes

src/FSharp.Core/map.fs Outdated Show resolved Hide resolved

no single line functions in FSHarp.Core

87c721a

dsyme force-pushed the ft15 branch from 5c97267 to 7ba9c76 Compare May 18, 2022 17:36

dsyme commented May 18, 2022

View reviewed changes

src/FSharp.Core/QueryExtensions.fs Outdated Show resolved Hide resolved

vzarytovskii reviewed May 18, 2022

View reviewed changes

src/FSharp.Core/map.fs Outdated Show resolved Hide resolved

Merge branch 'main' of https://github.com/dotnet/fsharp into ft15

22f61ef

dsyme force-pushed the ft15 branch from 7ba9c76 to 1505da9 Compare May 18, 2022 23:14

dsyme mentioned this pull request May 18, 2022

Remove spacing from array.fs #13164

Merged

Merge branch 'main' of https://github.com/dotnet/fsharp into ft15

1d29dd0

dsyme force-pushed the ft15 branch from 1505da9 to 24a5f51 Compare May 19, 2022 11:34

dsyme commented May 19, 2022

View reviewed changes

src/FSharp.Core/QueryExtensions.fs Show resolved Hide resolved

Merge branch 'main' of https://github.com/dotnet/fsharp into ft15

0f1e266

dsyme force-pushed the ft15 branch from 24a5f51 to 0f1e266 Compare May 20, 2022 10:27

merge main

cf35cad

dsyme force-pushed the ft15 branch from 4b84206 to e9e30bd Compare May 25, 2022 13:44

Merge branch 'main' of https://github.com/dotnet/fsharp into ft15

c04ad25

dsyme changed the title ~~WIP - format most of FSharp.Core~~ Format most of FSharp.Core May 27, 2022

dsyme force-pushed the ft15 branch from e9e30bd to d3f3a73 Compare May 27, 2022 15:57

update fantomas

ce5b517

vzarytovskii approved these changes May 30, 2022

View reviewed changes

Merge branch 'ft39' into ft15

7653d63

dsyme force-pushed the ft15 branch from d3f3a73 to 7653d63 Compare May 30, 2022 13:06

apply formatting

ece70f6

dsyme enabled auto-merge (squash) May 30, 2022 13:11

dsyme merged commit 05b560d into dotnet:main May 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format most of FSharp.Core #13150

Format most of FSharp.Core #13150

dsyme commented May 18, 2022 •

edited

Loading

vzarytovskii commented May 18, 2022

nojaf commented May 18, 2022

vzarytovskii May 18, 2022 •

edited

Loading

nojaf May 18, 2022

auduchinok May 18, 2022

nojaf May 18, 2022

dsyme May 18, 2022

dsyme May 18, 2022

dsyme May 19, 2022

dsyme commented May 18, 2022 •

edited

Loading

vzarytovskii May 18, 2022

dsyme May 18, 2022

nojaf May 18, 2022

vzarytovskii May 18, 2022

dsyme May 18, 2022

vzarytovskii May 18, 2022

nojaf May 18, 2022

dsyme May 18, 2022

dsyme commented May 19, 2022

dsyme commented May 19, 2022

nojaf commented May 19, 2022

dsyme commented May 19, 2022

		let inline NonStructural<'T when 'T: (static member (<): 'T * 'T -> bool) and 'T: (static member (>):
		'T * 'T -> bool)> : IComparer<'T> =

		type Event<'Delegate, 'Args when 'Delegate: delegate<'Args, unit> and 'Delegate :> System.Delegate and 'Delegate: not struct>
		() =

Format most of FSharp.Core #13150

Format most of FSharp.Core #13150

Conversation

dsyme commented May 18, 2022 • edited Loading

vzarytovskii commented May 18, 2022

nojaf commented May 18, 2022

vzarytovskii May 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsyme commented May 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsyme commented May 19, 2022

dsyme commented May 19, 2022

nojaf commented May 19, 2022

dsyme commented May 19, 2022

dsyme commented May 18, 2022 •

edited

Loading

vzarytovskii May 18, 2022 •

edited

Loading

dsyme commented May 18, 2022 •

edited

Loading