-
-
Notifications
You must be signed in to change notification settings - Fork 26
Support script (fsx) files #237
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
Conversation
…obal.json to deal with Attribute changes
… and script options
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 pull request introduces support for F# script files in the analyzer CLI while updating error codes and documentation to reflect the new functionality.
- Adds a new "Script" argument to accept .fsx files and implements glob-based file matching.
- Updates various exit points to use more descriptive error codes via the ExitErrorCodes enum.
- Updates documentation and package metadata accordingly.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/FSharp.Analyzers.Cli/Program.fs | Adds script file support, new error codes, and refines argument validation. |
| samples/BadOptionUsage.fsx | Introduces a sample script demonstrating potential OptionAnalyzer warnings. |
| global.json | Updates the SDK configuration with rollForward policy. |
| docs/index.md | Adds examples for running the CLI against script files. |
| Directory.Packages.props | Bumps the Argu package version. |
Comments suppressed due to low confidence (1)
src/FSharp.Analyzers.Cli/Program.fs:744
- [nitpick] Consider handling '--project' and '--script' conflicts with '--fsc-args' separately using distinct error codes or messages. This may provide clearer troubleshooting by identifying which mutually exclusive option is causing the conflict.
match fscArgs with
| <PackageVersion Include="Ionide.KeepAChangelog.Tasks" Version="0.1.8" PrivateAssets="all" /> | ||
| <PackageVersion Include="McMaster.NETCore.Plugins" Version="1.4.0" /> | ||
| <PackageVersion Include="Argu" Version="6.1.1" /> | ||
| <PackageVersion Include="Argu" Version="6.2.5" /> |
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.
Had to update Argu to deal with the Compiler attribute issue
| open FSharp.Analyzers.Cli.CustomLogging | ||
|
|
||
|
|
||
| type ExitErrorCodes = |
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.
Since we have exit 1 in like 100 places, I decided it's time to make exit codes specific to errors.
| if Path.IsPathRooted scriptGlob then | ||
| // Glob can't handle absolute paths, so we need to make sure the scriptGlob is a relative path | ||
| let root = Path.GetPathRoot scriptGlob | ||
| let glob = scriptGlob.Substring(root.Length) | ||
| DirectoryInfo root, glob | ||
| else if beginsWithCurrentPath scriptGlob then | ||
| // Glob can't handle relative paths starting with "./" or ".\", so we need trim it | ||
| let relativeGlob = scriptGlob.Substring(2) // remove "./" or ".\" | ||
| cwd, relativeGlob | ||
| else | ||
| cwd, scriptGlob |
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.
Some annoying path math
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.
If Glob doesn't support this stuff and hasn't been updated for some time, is it worth seeing if Microsoft.Extensions.FileSystemGlobbing would work? (as it's still updated, and doesn't have any depenencies)
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.
Ooo didn’t know this existed. Would gladly replace it.
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.
Ended up being a little more difficult because of how we curently use and I didn't want to spend that much time on it. Could be a good change for a subsequent PR.
|
This will probably conflict slightly with #236 but shouldn't be hard to fix either if one goes in first. |
0da7635 to
110b237
Compare
nojaf
left a comment
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.
Great stuff, well done!
Closes #234