Upgrade to fcs 39 - Githubissues

MangelMaxime commented 3 years ago

Related to #88

Note: It seems like there was a lot of white spaces in the original files and because my IDEs remove them it make a lot of noise in the diff.

It is possible in GitHub to hide the white space to make it easier to read the diff:

If that's really a problem, I could redo all the changes from master but I would prefer to just remove the white spaces from the files and add a .editorconfig file to ensure the behaviour across the different IDEs/developer machine. .editorconfig is file understand by most of the IDEs/Editors allowing to configure things like tabs size, removal of white space, etc.

Current status:

[X] Upgrade to FCS 39
[X] Change the project to target net5.0
~[ ] Change the path in the extension to use net5.0 instead of netcoreapp3.0 release~
[X] Fix warnings
- Warnings due to Array decomposition

- "Possible incorrect indentation"

[ ] There is a regression in tests/FSharpLanguageServer.Tests. The tests testing is .fsx files works is failing but it is not failing on master branch
[x] There is a failing test in tests/ProjectCracker.Tests but it is already failing in the master branch so not related to my changes. I will see if I can fix it or if I move directly to test Ionide/dotnet-proj library

This test starting failing when net5.0 has been introduced on the CI machine. On this build we can see that the Windows build is green while the Linux build is red.

The difference is Windows build doesn't have net5 while Linux does:

[x] I have a new warning when compiling the projects

fsharp-language-server\src\FSharpLanguageServer\FSharpLanguageServer.fsproj : warning NU1605: Detected package downgrade: FSharp.Core from 5.0.1 to 5.0.0. Reference the package directly from the project to select a different version. 
fsharp-language-server\src\FSharpLanguageServer\FSharpLanguageServer.fsproj : warning NU1605:  FSharpLanguageServer -> FSharp.Compiler.Service 39.0.0 -> FSharp.Core (= 5.0.1)
fsharp-language-server\src\FSharpLanguageServer\FSharpLanguageServer.fsproj : warning NU1605:  FSharpLanguageServer -> FSharp.Core (>= 5.0.0)

I don't know why I have this problem. I tried forcing FSharp.Core version in the fsproj but still get the warning.

When executing dotnet add package FSharp.Core, I have this error:

$ dotnet add src/FSharpLanguageServer/ package FSharp.Core
  Determining projects to restore...
  Writing C:\Users\Maxime\AppData\Local\Temp\tmpFC1C.tmp
info : Adding PackageReference for package 'FSharp.Core' into project 'C:\Users\Maxime\Documents\Workspace\Github\fsprojects\fsharp-language-server\src\FSharpLanguageServer\FSharpLanguageServer.fsproj'.
info :   CACHE https://api.nuget.org/v3/registration5-gz-semver2/fsharp.core/index.json
info : Restoring packages for C:\Users\Maxime\Documents\Workspace\Github\fsprojects\fsharp-language-server\src\FSharpLanguageServer\FSharpLanguageServer.fsproj...
info : Package 'FSharp.Core' is compatible with all the specified frameworks in project 'C:\Users\Maxime\Documents\Workspace\Github\fsprojects\fsharp-language-server\src\FSharpLanguageServer\FSharpLanguageServer.fsproj'.
error: Error while performing Update for package 'FSharp.Core'. Cannot edit items in imported files - 
error:   Item 'PackageReference' for 'FSharp.Core' in Imported file 'C:\Program Files\dotnet\sdk\5.0.202\FSharp\Microsoft.FSharp.NetSdk.props'.

georgewfraser commented 3 years ago

Hey @MangelMaxime this is still marked WIP---is this ready for review? Can you rebase on master?

I agree with you about .editorconfig

MangelMaxime commented 3 years ago

Hello @georgewfraser,

I guess it depends on what we want to ship. I think it should be possible to make a ready for review.

The only regression we have compared to the master branch is that .fsx support doesn't seems to work anymore.

Initially I wanted to also make the switch from custom project cracker to using dotnet-proj but I think I will make it in another PR. So we can have a first release for the FCS update.

I will try to make the required adjustment to the PR for a review this weekend.

MangelMaxime commented 3 years ago

Hello @georgewfraser,

I rebase this PR on master.

Compared to the master branch there is one failing which concern the .fsx support. I remove the WIP tag because I have no idea how to fix this issue and perhaps another contributor of this repo can send a PR to fix the issue.

When #92 is merged, it will be easier to read the diff because this PR depends on it.

fsprojects / fsharp-language-server

Upgrade to fcs 39 #93