Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix error linting projects that use .NET Framework #657

Merged
merged 7 commits into from
Jan 8, 2024

Conversation

webwarrior-ws
Copy link
Contributor

@webwarrior-ws webwarrior-ws commented Dec 28, 2023

  • Upgrade Ionide.ProjInfo to 0.58.0 (needed to bring the fix for parsing legacy projects, upstream PR: Support loading legacy .NET framework proj files ionide/proj-info#131 , upstream commit: ionide/proj-info@3542cee ).
  • Upgrade FSharp.Compiler.Service to version 41 (needed because the new Ionide.ProjInfo version, depends on a higher version of F.C.S.).
  • Made changes to framework, rules and test framework code necessary to accomodate API changes in this version 41.x of FCS compared to version 40.

Fixes #336

webwarrior-ws and others added 7 commits December 28, 2023 14:47
Upgraded FSharp.Compiler.Service to 41.0.1. Made changes to
framework, rules and test framework code necessary to
accomodate changes in FCS version 41 as compared to version 40.

FSharp.Compiler.Service 41.0.1 requires FSharp.Core 6.
Also had to pin Microsoft.Build version to 16.11.0 because of
build error. Fake uses 5.x uses FSharp.Core 5, so have to pin
it in Build group.

Had to pin Ionide.ProjInfo.* to version 0.53.0 (using ==
operator, which tells to use specific version instead of ~>
wihch allows the last part stated explicitly to increase) to
avoid the following error:
```
error NU1202: Package Ionide.ProjInfo 0.62.0 is not compatible with net5.0 (.NETCoreApp,Version=v5.0)
```

Updated paket.lock file using `dotnet paket update` command.

And then when executing `dotnet fake build` have errors like this:
```
C:\Users\PC\.nuget\packages\system.threading.tasks.dataflow\8.0.0\buildTransitive\netcoreapp2.0\System.Threading.Tasks.Dataflow.targets(4,5): error : System.Threading.Tasks.Dataflow 8.0.0 doesn't support net5.0 and has not been tested with it. Consider upgrading your TargetFramework to net6.0 or later. You may also set <SuppressTfmSupportBuildWarnings>true</SuppressTfmSupportBuildWarnings> in the project file to ignore this warning and attempt to run in this unsupported configuration at your own risk. [D:\Projects\work\FSharpLint\src\FSharpLint.Core\FSharpLint.Core.fsproj]
```

wip
Pin nunit version to 3.x because version 4 that gets selected
automatically now after upgrade to .net 6 has breaking changes
and breaks our tests.
In order to fix build process we pin the System.Reactive and
MSBuild.StructuredLogger version.
In order to use Visual Studio for building and debugging
FSharpLint, added `<NoWarn>$(NoWarn);NU1605</NoWarn>` to
Directory.Build.props file.

This has to be done because of version conflict as a result of
pinning of Microsoft.Build.* packages done in commit
57b4584 (Paket,Core,Tests: upgrade FCS to 41.0.1).

The errors due to version conflicts were like this:
```
Error NU1605 Warning As Error: Detected package downgrade: Microsoft.Build.Framework from 17.8.3 to 16.11.0. Reference the package directly from the project to select a different version.
 FSharpLint.Core -> Microsoft.Build.Tasks.Core 17.8.3 -> Microsoft.Build.Framework (>= 17.8.3)
 FSharpLint.Core -> Microsoft.Build.Framework (>= 16.11.0) FSharpLint.Core D:\Projects\work\FSharpLint\src\FSharpLint.Core\FSharpLint.Core.fsproj 1
```
Upgrade Ionide.ProjInfo package version to 0.58.0.

Fixes fsprojects#336.
Added new test for WildcardNamedWithAsPattern rule that checks
that wildcard pattern without as should not raise warning.
Fix rule WildcardNamedWithAsPattern that was broken when
porting to FCS version 41. Removed misleading comment.
@knocte
Copy link
Collaborator

knocte commented Dec 28, 2023

Upgrade FSharp.Compiler.Service to version 41.

Please explain why this is needed.

Upgrade Ionide.ProjInfo to 0.58.0.

Please explain why this is needed.

@knocte
Copy link
Collaborator

knocte commented Dec 28, 2023

Upgrade FCS to version 41 and Ionide.ProjInfo to 0.58.0

Title of PR should reflect the goal of the PR, not its implementation details.

@webwarrior-ws
Copy link
Contributor Author

Upgrade FSharp.Compiler.Service to version 41.

Please explain why this is needed.

Upgrade Ionide.ProjInfo to 0.58.0.

Please explain why this is needed.

Ionide.ProjInfo upgrade is needed to fix #336.
FSharp.Compiler.Service upgrade is needed because of Ionide.ProjInfo upgrade.

@webwarrior-ws webwarrior-ws changed the title Upgrade FCS to version 41 and Ionide.ProjInfo to 0.58.0 Fix error linting projects that use newer .NET versions Dec 28, 2023
@knocte
Copy link
Collaborator

knocte commented Dec 28, 2023

Ionide.ProjInfo upgrade is needed...

I'm not asking you to explain it to me, because I already know (and I know you know). I'm asking you to edit the PR description.

@knocte
Copy link
Collaborator

knocte commented Dec 28, 2023

Fix error linting projects that use newer .NET versions

Newer .NET versions??? Please read the github issue first.

@webwarrior-ws
Copy link
Contributor Author

Weren't there several issues that required new Ionide.ProjInfo? I remember testing on .NET 6 project.

@knocte
Copy link
Collaborator

knocte commented Dec 28, 2023

Weren't there several issues that required new Ionide.ProjInfo? I remember testing on .NET 6 project.

We didn't confirm that new versions of Ionide.ProjInfo fixed other problems. But you did confirm that it fixed issue #336, so that reason alone (only it) is enough to merge this PR.

@webwarrior-ws webwarrior-ws changed the title Fix error linting projects that use newer .NET versions Fix error linting projects that use .NET Framework Dec 28, 2023
@knocte knocte merged commit 6636fb2 into fsprojects:master Jan 8, 2024
5 checks passed
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.

Lint Not Working For .NET Framework Projects
3 participants