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

Adds exclusion config for analyzers #1120

Merged
merged 1 commit into from Jun 21, 2023

Conversation

TheAngryByrd
Copy link
Member

@TheAngryByrd TheAngryByrd commented Jun 18, 2023

WHAT

🤖 Generated by Copilot at ecc4c5a

This pull request enhances the configuration and progress reporting of the built-in analyzers in the FsAutoComplete project. It allows users to exclude files from analyzers based on regex patterns in the FSharpConfig settings, and adds progress indicators to the analyzer runs. It also improves some code quality and performance aspects in the AdaptiveFSharpLspServer type.

🤖 Generated by Copilot at ecc4c5a

FSharpConfig changed
Excluding files by regex
Winter of errors

🛠️🚫📊

WHY

Closes #1118

HOW

🤖 Generated by Copilot at ecc4c5a

  • Add fields and logic to handle regex patterns for analyzer exclusions in the F# configuration settings (link, link, link, link)
  • Use ServerProgressReport objects to report the progress of the analyzers to the LSP client (link, link, link)
  • Check the file path against the configuration settings before running each analyzer in the builtInCompilerAnalyzers function (link)
  • Define and use local values to simplify the code and avoid repeated calls in the builtInCompilerAnalyzers function (link)
  • Add open statements to import the System.Text.RegularExpressions and FsAutoComplete.Logging namespaces in LspHelpers.fs and AdaptiveFSharpLspServer.fs (link, link)
  • Add a helper function tryCreateRegex to handle the possible failures of creating Regex objects from strings in ClassificationUtils.fs (link)

Comment on lines +266 to +267
use progress = new ServerProgressReport(lspClient)
do! progress.Begin("Checking unused opens...", message = filePathUntag)
Copy link
Member Author

Choose a reason for hiding this comment

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

Adds ProgressReport for users to help identify files which might benefit from disabling.

let analyzers =
[
// if config.Linter then
// commands.Lint filePath |> Async .Ignore
if config.UnusedOpensAnalyzer then
if config.UnusedOpensAnalyzer && isNotExcluded config.UnusedOpensAnalyzerExclusions then
Copy link
Member Author

Choose a reason for hiding this comment

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

Exclusion check

@@ -759,8 +777,11 @@ type FSharpConfig =
InterfaceStubGenerationMethodBody: string
AddPrivateAccessModifier: bool
UnusedOpensAnalyzer: bool
UnusedOpensAnalyzerExclusions: Regex array
Copy link
Member Author

Choose a reason for hiding this comment

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

Since we already have FSharp.unusedOpensAnalyzer it would be a breaking change to make it a nested object and all client configs would need to update for this. Could be renamed to whatever makes more sense though.

@@ -738,6 +743,19 @@ type DebugConfig =
LogDurationBetweenCheckFiles = false
LogCheckFileDuration = false }

let logger = lazy (LogProvider.getLoggerByName "LspHelpers")

let tryCreateRegex (pattern: string) =
Copy link
Member Author

Choose a reason for hiding this comment

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

Safe creation of these Regex since the config could be bad regex.

@baronfel
Copy link
Contributor

This makes sense to me - but it makes me wonder if we need a more general config for each analyzer:

"FSharp.analyzers.simplifyNames.enabled": true,
"FSharp.analyzers.simplifyNames.ignorePatterns": ["*.g.fs", ... ]

Which you brought up, and would be a large change, but I just wanted to signal that I agree with that thought overall and think we should go that direction eventually.

@TheAngryByrd
Copy link
Member Author

TheAngryByrd commented Jun 20, 2023

Which you brought up, and would be a large change, but I just wanted to signal that I agree with that thought overall and think we should go that direction eventually.

I guess we could have a JsonConverter to do the magic of upgrading old configs.

I'm bad at JsonConverters, just gonna go with this.

@TheAngryByrd TheAngryByrd merged commit 3b20177 into ionide:main Jun 21, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add glob/regex ignore for built in analyzers
2 participants