Draft: Implement proper rule overriding to address issue #488

[BennieCopeland]

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]

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]

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]

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

Missing on the Configuration type

• unnestedFunctionNames
• nestedFunctionNames

Missing in the JSON file

• NonPublicValuesNames I see this has been deprecated

[BennieCopeland]

Pretty sure these lines are not correct for creating the NonPublicValuesNames rules:

FSharpLint/src/FSharpLint.Core/Application/Configuration.fs

Lines 711 to 712 in 873d145

	config.NonPublicValuesNames |> Option.bind (constructRuleWithConfig PrivateValuesNames.rule)
	config.NonPublicValuesNames |> Option.bind (constructRuleWithConfig InternalValuesNames.rule)

I noticed after the fact that this has been deprecated.

[BennieCopeland]

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

• UnnestedFunctionNames
• NestedFunctionNames
• SuggestUseAutoProperty

[knocte]

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]

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

[knocte]

@BennieCopeland sorry for the huge delay Bennie and thanks for your work, do you mind rebasing? Looks like CI would be fixed just by doing that.

[knocte]

@BennieCopeland thanks for taking a look at this again! But I think you forgot to push a commit in your latest rebase?

[BennieCopeland]

@knocte Yes, I had some more unfinished work stashed, but it's been so long since I've looked at this that I'm having to reconstruct what exactly I what I was trying to do. I'll start looking at this again and see what I need to do to finish it up.