Skip to content

Conversation

@dsyme
Copy link
Contributor

@dsyme dsyme commented Feb 19, 2019

The lines in the F# compiler code are waaaaaaay too long. It's a huge problem for code readability, e.g. when searching the codebase and landing on column 250 of some file. The longest line in master is around 400 characters.

This PR reformats about 100 massive lines in the range 250-400 characters and a bunch of other lines in the "huge" category. After this the longest line in src\fsharp is 250. (There are 460+ longer lines in absil but I haven't touched those yet).

It also adds a script that checks line lengths, current output after this change is below. I've added some exceptions for big tables in TcGlobals and tast.fs.

fsi tests\scripts\longLines.fsx

Perhaps a good community goal to get maximum line length down to around 150 (my laptop shows about 150), which would mean reformatting about 1500 lines. It's likely some of these could be skipped.

6676 long lines = 0.05%
1529 huge lines = 0.01%
337 humungous lines = 0.00%

bucket 250-259 - %0.0
bucket 240-249 - %0.0
bucket 230-239 - %0.0
bucket 220-229 - %0.0
bucket 210-219 - %0.1
bucket 200-209 - %0.1
bucket 190-199 - %0.1
bucket 180-189 - %0.2
bucket 170-179 - %0.3
bucket 160-169 - %0.4
bucket 150-159 - %0.6
bucket 140-149 - %0.8
bucket 130-139 - %1.1
bucket 120-129 - %1.8
bucket 110-119 - %2.3
bucket 100-109 - %3.3
bucket 90-99 - %4.1
bucket 80-89 - %5.2
bucket 70-79 - %7.4
bucket 60-69 - %7.5
bucket 50-59 - %8.9
bucket 40-49 - %10.5
bucket 30-39 - %11.8
bucket 20-29 - %10.8
bucket 10-19 - %6.0
bucket 0-9 - %16.7

@dsyme
Copy link
Contributor Author

dsyme commented Feb 19, 2019

This is now ready

@dsyme
Copy link
Contributor Author

dsyme commented Feb 19, 2019

+1200 lines is an improvement in this case :)

@dsyme
Copy link
Contributor Author

dsyme commented Feb 20, 2019

Would appreciate an "approve" on this so we can just merge it, it's just code sanitization

@cartermp
Copy link
Contributor

Test failure:

Failed   TypeProvider.Disposal.SmokeTest1
Error Message:
   Check1, countDisposals() < i, iteration 23
  Expected: True
  But was:  False

Stack Trace:
   at Tests.LanguageService.Script.UsingMSBuild.TypeProviderDisposalSmokeTest(Boolean clearing) in D:\a\1\s\vsintegration\tests\UnitTests\LegacyLanguageService\Tests.LanguageService.Script.fs:line 1615
   at Tests.LanguageService.Script.UsingMSBuild.TypeProvider.Disposal.SmokeTest1() in D:\a\1\s\vsintegration\tests\UnitTests\LegacyLanguageService\Tests.LanguageService.Script.fs:line 1649

@dsyme
Copy link
Contributor Author

dsyme commented Feb 20, 2019

@cartermp That's a flaky test too. #6255. I believe (without being certain) that these are due to tests being run in parallel

@cartermp
Copy link
Contributor

Sounds good. Although I'm personally fine with merging, I just don't want @KevinRansom and @brettfo to have too much of a headache when we merge 16.0 -> master when VS 2019 is GA.

@dsyme dsyme closed this Feb 20, 2019
@dsyme dsyme reopened this Feb 20, 2019
@dsyme
Copy link
Contributor Author

dsyme commented Feb 20, 2019

MacOS test machine flaked out

Copy link
Contributor

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long lines is just natures way of telling you to get a bigger monitor :-)

@KevinRansom KevinRansom merged commit 78c7b17 into dotnet:master Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants