search

fsprojects / FSharpLint

Lint tool for F#
https://fsprojects.github.io/FSharpLint/
MIT License
303 stars 73 forks source link

Draft: Implement proper rule overriding to address issue #488 #711

Open BennieCopeland opened 3 months ago

BennieCopeland commented 3 months ago

I'm looking to add proper overriding back in. I expect that this will be a breaking change as the current functionality appears to allow running no rules by providing an empty JSON object. The new functionality will run all default enabled rules unless explicitly disabled in a custom config.

For the implementation, there are a few different ways I've considered that require discussion.

  1. Keep the default rules in a JSON file, or rewrite them as F# code?

    • JSON File Pros

      • Can be used as an example for providing overrides
      • Single location for changing rules

    • JSON File Cons

      • Default rules are not co-located with the code implementing the rules
      • Code implementing the rules and the settings can become out of sync

    • F# Code Pros

      • Defaults can be co-located with the rules making them easier to keep updated with implementation changes
      • Out of sync defaults becomes a compile time issue
      • Splits the internal and external representations. Right now there there is a single Configuration type that includes deprecated settings. Splitting into Configuration and Override types can keep the internal code cleaner/more focused

    • F# Code Cons

      • Not as easy to show an example of a valid JSON config, but this could be mitigated by including an example config that is used by the FSharp.Data Json type provider. This will allow providing an example overrides file that also stays synchronized with the code base

    Whether the rules are kept in JSON or F#, adding/removing rules or changing defaults results in a new build and version bump.

  2. Continue to use the same Configuration type, or rename Configuration to Overrides and create a new Configuration type without optional to reflect the default config.

BennieCopeland commented 3 months ago

For the JSON config, if there are both deprecated configs and non-deprectated configs that do the same thing, which one take priority? Or, should this be the time to remove the deprectated settings?

BennieCopeland commented 3 months ago

Thinking about it, if the config is in F#, a new command line argument can be added that JSON serializes the type to the screen or a file for someone to start with.

BennieCopeland commented 3 months ago

While comparing the fsharplint.json file to the Configuration type, I found some discrepancies.

Missing on the Configuration type

Missing in the JSON file

BennieCopeland commented 3 months ago

~Pretty sure these lines are not correct for creating the NonPublicValuesNames rules:~ https://github.com/fsprojects/FSharpLint/blob/873d1452a0151f5541ff5470d7558cd0d6edd036/src/FSharpLint.Core/Application/Configuration.fs#L711-L712

I noticed after the fact that this has been deprecated.

BennieCopeland commented 3 months ago

The following rules are NOT part of the allRules array when flattening the config and thus never being ran:

knocte commented 2 months ago

Hi Bennie, thanks for your work so far! Sorry for the delay replying, life is kind of hectic nowadays for me.

The following rules are NOT part of the allRules array when flattening the config and thus never being ran:

Do you mind opening a separate PR to fix this please?

BennieCopeland commented 2 months ago

Sure, new PR for that change here: #713.