Apply standard source code formatting to src/fsharp/*.fsi #12346

dsyme · 2021-11-05T22:05:01Z

Some low hanging fruit in applying standard source code formatting is to format all our signature files.

git checkout ft6
git fetch home
git reset --hard home/main
git merge 5c091ba8f
dotnet tool restore
dotnet fantomas src\fsharp
git commit -a -m "apply formatting"
git push -f origin ft6

There's two bits of formatting I don't like

int [] for arrays (not the extra space, which fantomas adds, though we should likely switch to use int array instead anyway, the community have requested that be considered normal)
Overly verbose formatting of some attributes

However we can still merge this

Some files fail formatting, see src\fsharp.fantomasignore. I raised two issues about those so far

dsyme · 2021-11-05T23:49:19Z

@KevinRansom @TIHan @brettfo @vzarytovskii This is ready.

We won't run formatting checks as standard for checkin but will add it soon. And note this is only the signature files so far. The implementation files will be much more challenging

Some reformatting will occur with future versions of Fantomas as we continue to improve the defaults.

dsyme · 2021-11-06T00:05:32Z

Converting to draft as this may conflict badly with merges between main and dev17.0.

We may also have conflicts with outstanding PRs such as for nullable reference types and #12265 and #11681 and #6805. In theory should be possible to simply re-run formatting on those PRs before merging together. But we have to be careful not to create unnecessary pain for ourselves by doing this at the wrong time.

Anyway I think we should wait at least until dev17.0 is merged in. It's really easy to run the formatting again so it's no problem to wait.

nojaf · 2021-11-20T08:38:45Z

Hello @dsyme, fsprojects/fantomas#1953 and fsprojects/fantomas#1954 have been fixed.
They are part of the 4.6.0-alpha-008 release.
I'm not sure if you can just update if newer versions are present on NuGet.
So, perhaps you want to update in another PR.

Out of interest, did you format the formatted files to check if they are idempotent?

dsyme · 2021-11-20T12:28:52Z

I'm not sure if you can just update if newer versions are present on NuGet.

Thanks! We have to ask a team at microsoft to put new packages up, about 24h turnaround, I've done that now

dsyme · 2021-11-20T12:29:33Z

fsprojects/fantomas#1953 and fsprojects/fantomas#1954 have been fixed.

This is great! I was actually going to work on those :) Thank you, I'll try to make it up to you by fixing other things :)

dsyme · 2021-11-20T12:37:25Z

@nojaf There was one case of non-idempotency, but it's poor input code so I don't mind changing it. Do you want me to report it?

    type ProviderGeneratedType = ProviderGeneratedType of (*ilOrigTyRef*)ILTypeRef * (*ilRenamedTyRef*)ILTypeRef * ProviderGeneratedType list

goes to

    type ProviderGeneratedType =
        | ProviderGeneratedType of ILTypeRef (*ilRenamedTyRef*)  * ILTypeRef * ProviderGeneratedType list (*ilOrigTyRef*)

then to

    type ProviderGeneratedType =
        | ProviderGeneratedType of
            ILTypeRef (*ilRenamedTyRef*)  *
            ILTypeRef *
            ProviderGeneratedType list (*ilOrigTyRef*)

I'm just going to change the original code to

    type ProviderGeneratedType = ProviderGeneratedType of ilOrigTyRef: ILTypeRef * ilRenamedTyRef: ILTypeRef * ProviderGeneratedType list

dsyme · 2021-11-20T12:44:52Z

@nojaf there is one other non-idempotency, reported here: fsprojects/fantomas#1974

nojaf · 2021-11-20T12:57:13Z

Do you want me to report it?

Yes, it is an interesting case where we at first glance, don't print trivia. Please report it and mention it is a low priority.
Thanks!

dsyme · 2021-11-20T14:16:20Z

This is ready. Now that dev17.0 has been integrated into main it' a goiod time to do this.

KevinRansom

@dsyme Why do we have tuple arguments with the * at the start of the line rather than the end?

src/fsharp/CheckDeclarations.fsi

KevinRansom · 2021-11-22T21:20:14Z

src/fsharp/CheckFormatStrings.fsi

-    -> printerResidueTy: TType
-    -> printerResultTy: TType
-        -> TType list * TType * TType * TType[] * (range * int) list * string
+    m: range ->


Interestingly, here the format is how I like it.

Yup, we have the -> at the end OK. Full context is linked in https://github.com/fsprojects/fantomas/issues/1955. THe problem is in mixed curried/tupled signatures but we rarely use those

dsyme · 2021-11-25T12:15:43Z

@dsyme you might also want to add a ignore revs file once this is ready. See https://www.moxio.com/blog/43/ignoring-bulk-change-commits-with-git-blame

Oh wow, thank you for this! That's so useful. I guess we should be using this for all cleanup PRs too

dsyme · 2022-04-20T03:10:18Z

I have updated this to fantomas-tool 4.7.5 but am getting an error:

Processing .\range.fsi
The following exception occurs while formatting stdin: System.Exception: Fantomas is trying to format the input multiple times due to the detect of multiple defines.
There is a problem with merging all the code back together.
[CHECK_LINE0_TYPES] has 4 fragments
[] has 2 fragments

dsyme · 2022-04-20T03:29:43Z

@vzarytovskii @KevinRansom I've updated things to include a "CheckCodeFormatting" CI job to see if code formatting is up-to-date in the formatted files (which are primarily just signature files)

Once green I think we can soon go ahead with merging this as first steps? Then iteratively

start to apply code formatting to more files
improve our signature files to include more metadata, better comments, less use of mixed tuple/curried signatures.
improve fantomas

dsyme · 2022-04-28T04:28:41Z

@vzarytovskii @KevinRansom Can you approve if you agree please? Thanks

vzarytovskii

Looks good to me, perhaps we can do some tests next.
@KevinRansom does it look okay to you?

dsyme · 2022-04-30T08:26:07Z

Looks good to me, perhaps we can do some tests next.

Sounds good to me (though beware sometimes the data expressions in tests are massively expanded by fantomas formatting)

I'll update this

Issue in review has been addressed

dsyme · 2022-05-04T13:06:09Z

@vzarytovskii @KevinRansom @nojaf OK, Fantomas is now being used to format the signature files in this repository

nojaf · 2022-05-04T13:14:47Z

azure-pipelines.yml

+            env:
+              DOTNET_ROLL_FORWARD_TO_PRERELEASE: 1
+            displayName: Install tools
+          - script: dotnet fantomas src\fsharp --check


@dsyme sorry, I didn't catch this, but I don't think the \ will work on a Linux VM.
I don't believe the check will execute properly.

Interesting

I did have then check fail, my understanding is it now succeeds. Odd

auduchinok · 2022-05-08T13:29:35Z

src/fsharp/CompilerImports.fsi

-exception AssemblyNotResolved of (*originalName*) string * range
+exception AssemblyNotResolved of string * range (*originalName*)


@nojaf Have you seen changes related to comments like this and ones below?

Nope, I didn't really review the changes in this PR.

auduchinok · 2022-05-08T14:44:18Z

src/fsharp/BinaryResourceFormats.fsi

-          -> byte[]
-
+        seq<int32 * int32> ->
+            byte []


Looks like another place worth fixing (if not yet 🙂)

What's wrong here? Did you catch up on https://docs.microsoft.com/en-us/dotnet/fsharp/style-guide/formatting#formatting-types-in-signatures?

It was about the added space.

The space between the byte and [] has been resolved. Available in the v5 alphas.
The space before byte is part of the indentation of the return type.

Cool, thanks for confirming!

auduchinok · 2022-05-08T14:46:12Z

src/fsharp/ConstraintSolver.fsi

-                        * candidates: OverloadInformation list // methodNames may be different (with operators?), this is refactored from original logic to assemble overload failure message
-                        * cx: TraitConstraintInfo option
+    | NoOverloadsFound of methodName: string * candidates: OverloadInformation list * cx: TraitConstraintInfo option
+    | PossibleCandidates of


Another place where an empty line between multiline declarations would look great (as discussed in fsprojects/fantomas#2198).

Could be pitched in the style guide.

* add fantomasignore * add fantomasignore * fix ignores * fix ignores * update fantomasignore * prefix before formatting * prefix before formatting * bump fantomas version * update tool * update tool * update tool * update tool * add job * add job * apply formatting

dsyme added 2 commits November 5, 2021 22:09

add fantomasignore

3e4500d

add fantomasignore

5d52552

dsyme force-pushed the ft6 branch from a415348 to 6b99b3b Compare November 5, 2021 22:13

fix ignores

bba52dd

dsyme force-pushed the ft6 branch from 6c99b19 to 07a8803 Compare November 5, 2021 22:42

fix ignores

2c40cda

dsyme force-pushed the ft6 branch from 07a8803 to bd9bc83 Compare November 5, 2021 23:16

dsyme enabled auto-merge (squash) November 5, 2021 23:47

dsyme disabled auto-merge November 6, 2021 00:02

dsyme marked this pull request as draft November 6, 2021 00:02

dsyme marked this pull request as ready for review November 20, 2021 01:48

dsyme force-pushed the ft6 branch 2 times, most recently from 7a75d8b to 44caca0 Compare November 20, 2021 01:55

dsyme added 2 commits November 20, 2021 12:33

Merge commit '2c40cda21f90e9a2e1f65639956885a73a6e1b88' into ft6

e72e7f8

update fantomasignore

06a9dbb

prefix before formatting

e073f8d

prefix before formatting

1944274

dsyme force-pushed the ft6 branch from 44caca0 to 83d30fa Compare November 20, 2021 12:51

bump fantomas version

8f53151

dsyme force-pushed the ft6 branch from 83d30fa to 630eed6 Compare November 20, 2021 13:11

KevinRansom previously requested changes Nov 22, 2021

View reviewed changes

dsyme mentioned this pull request Dec 2, 2021

[style-guide] Reconsidering long tupled signatures fsharp/fslang-design#644

Closed

dsyme added 2 commits April 20, 2022 12:54

Merge commit '8f53151eaf1c6070f330a8c08a4701a0f8abb00c' into ft6

3591e40

update tool

531b82f

dsyme force-pushed the ft6 branch from 630eed6 to 531b82f Compare April 20, 2022 03:05

update tool

cd56e17

dsyme added 2 commits April 20, 2022 13:11

update tool

1e217a3

update tool

8f9b90f

dsyme force-pushed the ft6 branch from 02f6d9f to e6a0f68 Compare April 20, 2022 03:26

add job

5694db4

dsyme force-pushed the ft6 branch from e6a0f68 to f25aa78 Compare April 20, 2022 03:33

add job

5c091ba

dsyme force-pushed the ft6 branch from f25aa78 to aaf91e4 Compare April 20, 2022 03:37

vzarytovskii approved these changes Apr 29, 2022

View reviewed changes

dsyme force-pushed the ft6 branch from aaf91e4 to e8f6a4c Compare April 30, 2022 08:48

dsyme added 2 commits May 2, 2022 16:39

Merge commit '5c091ba8f' into ft6

1765b88

apply formatting

1f8a7ce

dsyme force-pushed the ft6 branch from e8f6a4c to 1f8a7ce Compare May 2, 2022 15:40

dsyme merged commit f5e7428 into dotnet:main May 4, 2022

nojaf reviewed May 4, 2022

View reviewed changes

auduchinok reviewed May 8, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply standard source code formatting to src/fsharp/*.fsi #12346

Apply standard source code formatting to src/fsharp/*.fsi #12346

dsyme commented Nov 5, 2021 •

edited

Loading

dsyme commented Nov 5, 2021 •

edited

Loading

dsyme commented Nov 6, 2021 •

edited

Loading

nojaf commented Nov 20, 2021

dsyme commented Nov 20, 2021

dsyme commented Nov 20, 2021

dsyme commented Nov 20, 2021 •

edited

Loading

dsyme commented Nov 20, 2021

nojaf commented Nov 20, 2021

dsyme commented Nov 20, 2021 •

edited

Loading

KevinRansom left a comment

KevinRansom Nov 22, 2021

dsyme Nov 22, 2021

dsyme commented Nov 25, 2021

dsyme commented Apr 20, 2022

dsyme commented Apr 20, 2022 •

edited

Loading

dsyme commented Apr 28, 2022

vzarytovskii left a comment

dsyme commented Apr 30, 2022

dsyme commented May 4, 2022

nojaf May 4, 2022

dsyme May 4, 2022

dsyme May 4, 2022

auduchinok May 8, 2022

nojaf May 9, 2022

auduchinok May 8, 2022

nojaf May 9, 2022

auduchinok May 9, 2022

nojaf May 9, 2022 •

edited

Loading

auduchinok May 9, 2022

auduchinok May 8, 2022

nojaf May 9, 2022

		exception AssemblyNotResolved of (originalName) string * range
		exception AssemblyNotResolved of string * range (originalName)

Apply standard source code formatting to src/fsharp/*.fsi #12346

Apply standard source code formatting to src/fsharp/*.fsi #12346

Conversation

dsyme commented Nov 5, 2021 • edited Loading

dsyme commented Nov 5, 2021 • edited Loading

dsyme commented Nov 6, 2021 • edited Loading

nojaf commented Nov 20, 2021

dsyme commented Nov 20, 2021

dsyme commented Nov 20, 2021

dsyme commented Nov 20, 2021 • edited Loading

dsyme commented Nov 20, 2021

nojaf commented Nov 20, 2021

dsyme commented Nov 20, 2021 • edited Loading

KevinRansom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsyme commented Nov 25, 2021

dsyme commented Apr 20, 2022

dsyme commented Apr 20, 2022 • edited Loading

dsyme commented Apr 28, 2022

vzarytovskii left a comment

Choose a reason for hiding this comment

dsyme commented Apr 30, 2022

dsyme commented May 4, 2022

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

nojaf May 9, 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

dsyme commented Nov 5, 2021 •

edited

Loading

dsyme commented Nov 5, 2021 •

edited

Loading

dsyme commented Nov 6, 2021 •

edited

Loading

dsyme commented Nov 20, 2021 •

edited

Loading

dsyme commented Nov 20, 2021 •

edited

Loading

dsyme commented Apr 20, 2022 •

edited

Loading

nojaf May 9, 2022 •

edited

Loading