-
Notifications
You must be signed in to change notification settings - Fork 15
Update to FSharpLint.Core v0.26.2 (latest) #108
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
base: master
Are you sure you want to change the base?
Conversation
- Update .NET SDK to 9.0 - Update F# to 9.0 - Update FSharpLint.Core to 0.26.2 (latest) - Update all dependencies to latest
|
||
<PropertyGroup> | ||
<OutputType>Exe</OutputType> | ||
<TargetFramework>net50</TargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to Directory.Build.props
<!-- see https://docs.microsoft.com/en-us/dotnet/core/rid-catalog for RIds --> | ||
<RuntimeIdentifiers>win-x64;osx-x64;linux-x64</RuntimeIdentifiers> | ||
<ProjectGuid>504f4169-b41d-431f-b31e-e72e0fe2495c</ProjectGuid> | ||
<PublishTrimmed>true</PublishTrimmed> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment in Directory.Build.props
| Some h -> | ||
match h.add with | ||
| Some a -> Assert.AreEqual(108, a.Length) | ||
| Some a -> Assert.That(a.Length, Is.EqualTo(109)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines Of Code is one greater than before for some reason
Changed PR status to "draft" since Java rules need to be updated too |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the project from .NET 5.0 to .NET 9.0 and upgrades FSharpLint.Core from v0.16.5 to v0.26.2, which introduces significant breaking changes in the F# Compiler Services API. The update adds support for new lint rules and modernizes the codebase.
Key changes:
- Upgrade .NET SDK from 5.0.101 to 9.0.301 and F# to 9.0
- Update FSharpLint.Core to v0.26.2 with 20 new rules (from 55 to 75 total rules)
- Migrate from deprecated F# Compiler Services namespaces to new ones (SourceCodeServices → CodeAnalysis, SyntaxTree → Syntax, Range → Text)
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
FsSonarRunner/rules.txt | Updates rule list with 20 new FSharpLint rules and fixes typo |
FsSonarRunner/global.json | Updates .NET SDK version to 9.0.301 |
FsSonarRunnerCore/UntypeAST.fs | Major refactoring to use new F# Compiler Services API with updated AST pattern matching |
FsSonarRunnerCore/SQAnalyser.fs | Updates namespace imports and fixes function calls for new API |
FsSonarRunnerCore/Rules/*.fs | Adds configuration for new lint rules and updates rule mappings |
Project files | Updates target framework to .NET 9.0 and dependency versions |
Test files | Updates test expectations and fixes compatibility issues |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|> Option.defaultValue "" | ||
|> System.Net.WebUtility.HtmlDecode | ||
|> fun x -> x.Split(separator) | ||
|> function null -> [||] | x -> x.Split(separator) |
Copilot
AI
Sep 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This null-check pattern using function
is unnecessarily complex. Consider using the isNull
function or Option.ofObj for better readability: |> fun x -> if isNull x then [||] else x.Split(separator)
|> function null -> [||] | x -> x.Split(separator) | |
|> Option.ofObj | |
|> Option.map (fun x -> x.Split(separator)) | |
|> Option.defaultValue [||] |
Copilot uses AI. Check for mistakes.
let rec solutionDirectory = | ||
function | ||
| Some path -> | ||
if Path.Combine(path, "FsSonarRunner.sln") |> File.Exists | ||
then path | ||
else Path.GetDirectoryName(path) |> Option.ofObj |> solutionDirectory | ||
| _ -> | ||
failwith "Solution directory not found" |
Copilot
AI
Sep 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The recursive function could potentially cause a stack overflow if the directory structure is deeply nested and no solution file is found. Consider adding a depth limit or using a tail-recursive approach with an accumulator.
Copilot uses AI. Check for mistakes.
failwith "Solution directory not found" | ||
|
||
let runningPath = | ||
Assembly.GetExecutingAssembly().Location.Replace("file:///", "") |
Copilot
AI
Sep 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string replacement Replace(\"file:///\", \"\")
is likely incorrect for assembly locations. Assembly.Location returns a file path, not a URI, so this replacement may not be necessary and could cause issues on different platforms.
Assembly.GetExecutingAssembly().Location.Replace("file:///", "") | |
Assembly.GetExecutingAssembly().Location |
Copilot uses AI. Check for mistakes.
FSharpLint.Core
v0.26.2 supports .NET 8.0 and 9.0, so I choose to update this project to 9.0.Everything is working AFAICT, but please carefully review
FsSonarRunnerCore\UntypeAST.fs
as there are some significant changes. AlsoFsSonarRunnerCore\Rules\*.fs
.