Default rules for v1

[jgardella]

What should the default rules be for FSharpLint? We can update them as part of the v1 release.

I would like them to align with the F# style guide as much as possible, but if there are points of contention there we can work to update the guide to be aligned with community opinion first, and then set the default rules based on that.

[jgardella]

My initial proposal (omitting hints):

One point of contention might be the typedItemSpacing rule being enabled, or the default setting. The rule checks the spacing around the colon in code like: x : int, and has three settings: SpacesAround, SpaceAfter, or NoSpace.

{
    "ignoreFiles": [
        "assemblyinfo.*"
    ],
    "global": {
        "numIndentationSpaces": 4
    },
    "typedItemSpacing": {
        "enabled": true,
        "config": {
            "typedItemStyle": "SpacesAround"
        }
    },
    "typePrefixing": { "enabled": true },
    "unionDefinitionIndentation": { "enabled": true },
    "moduleDeclSpacing": { "enabled": false },
    "classMemberSpacing": { "enabled": false },
    "tupleCommaSpacing": { "enabled": true },
    "tupleIndentation": { "enabled": true },
    "tupleParentheses": { "enabled": true },
    "patternMatchClausesOnNewLine": { "enabled": false },
    "patternMatchOrClausesOnNewLine": { "enabled": false },
    "patternMatchClauseIndentation": { "enabled": true },
    "patternMatchExpressionIndentation": { "enabled": true },
    "recursiveAsyncFunction": { "enabled": true },
    "redundantNewKeyword": { "enabled": true },
    "nestedStatements": {
        "enabled": false,
        "config": {
            "depth": 8
        }
    },
    "reimplementsFunction": { "enabled": true },
    "canBeReplacedWithComposition": { "enabled": true },
    "failwithWithSingleArgument": { "enabled": true },
    "raiseWithSingleArgument": { "enabled": true },
    "nullArgWithSingleArgument": { "enabled": true },
    "invalidOpWithSingleArgument": { "enabled": true },
    "invalidArgWithTwoArguments": { "enabled": true },
    "failwithfWithArgumentsMatchingFormatString": { "enabled": true },
    "maxLinesInLambdaFunction": {
        "enabled": false,
        "config": {
            "maxLines": 7
        }
    },
    "maxLinesInMatchLambdaFunction": {
        "enabled": false,
        "config": {
            "maxLines": 100
        }
    },
    "maxLinesInValue": {
        "enabled": false,
        "config": {
            "maxLines": 100
        }
    },
    "maxLinesInFunction": {
        "enabled": false,
        "config": {
            "maxLines": 100
        }
    },
    "maxLinesInMember": {
        "enabled": false,
        "config": {
            "maxLines": 100
        }
    },
    "maxLinesInConstructor": {
        "enabled": false,
        "config": {
            "maxLines": 100
        }
    },
    "maxLinesInProperty": {
        "enabled": false,
        "config": {
            "maxLines": 70
        }
    },
    "maxLinesInModule": {
        "enabled": false,
        "config": {
            "maxLines": 1000
        }
    },
    "maxLinesInRecord": {
        "enabled": false,
        "config": {
            "maxLines": 500
        }
    },
    "maxLinesInEnum": {
        "enabled": false,
        "config": {
            "maxLines": 500
        }
    },
    "maxLinesInUnion": {
        "enabled": false,
        "config": {
            "maxLines": 500
        }
    },
    "maxLinesInClass": {
        "enabled": false,
        "config": {
            "maxLines": 500
        }
    },
    "interfaceNames": {
        "enabled": true,
        "config": {
            "naming": "PascalCase",
            "underscores": "None",
            "prefix": "I"
        }
    },
    "exceptionNames": {
        "enabled": true,
        "config": {
            "naming": "PascalCase",
            "underscores": "None",
            "suffix": "Exception"
        }
    },
    "typeNames": {
        "enabled": true,
        "config": {
            "naming": "PascalCase",
            "underscores": "None"
        }
    },
    "recordFieldNames": {
        "enabled": true,
        "config":  {
            "naming": "PascalCase",
            "underscores": "None"
        }
    },
    "enumCasesNames": {
        "enabled": true,
        "config": {
            "naming": "PascalCase",
            "underscores": "None"
        }
    },
    "unionCasesNames": {
        "enabled": true,
        "config": {
            "naming": "PascalCase",
            "underscores": "None"
        }
    },
    "moduleNames": {
        "enabled": true,
        "config": {
            "naming": "PascalCase",
            "underscores": "None"
        }
    },
    "literalNames": {
        "enabled": true,
        "config": {
            "naming": "PascalCase",
            "underscores": "None"
        }
    },
    "namespaceNames": {
        "enabled": true,
        "config": {
            "naming": "PascalCase",
            "underscores": "None"
        }
    },
    "memberNames": {
        "enabled": true,
        "config": {
            "naming": "PascalCase",
            "underscores": "AllowPrefix"
        }
    },
    "parameterNames": {
        "enabled": true,
        "config": {
            "naming": "CamelCase",
            "underscores": "AllowPrefix"
        }
    },
    "measureTypeNames": {
        "enabled": true,
        "config": {
            "underscores": "None"
        }
    },
    "activePatternNames": {
        "enabled": true,
        "config": {
            "naming": "PascalCase",
            "underscores": "None"
        }
    },
    "publicValuesNames": {
        "enabled": true,
        "config": {
            "underscores": "AllowPrefix"
        }
    },
    "nonPublicValuesNames": {
        "enabled": true,
        "config": {
            "naming": "CamelCase",
            "underscores": "AllowPrefix"
        }
    },
    "maxNumberOfItemsInTuple": {
        "enabled": false,
        "config": {
            "maxItems": 4
        }
    },
    "maxNumberOfFunctionParameters": {
        "enabled": false,
        "config": {
            "maxItems": 5
        }
    },
    "maxNumberOfMembers": {
        "enabled": false,
        "config": {
            "maxItems": 32
        }
    },
    "maxNumberOfBooleanOperatorsInCondition": {
        "enabled": false,
        "config": {
            "maxItems": 4
        }
    },
    "favourIgnoreOverLetWild": { "enabled": true },
    "wildcardNamedWithAsPattern": { "enabled": true },
    "uselessBinding": { "enabled": true },
    "tupleOfWildcards": { "enabled": true },
    "indentation": {
        "enabled": true
    },
    "maxCharactersOnLine": {
        "enabled": false,
        "config": {
            "maxCharactersOnLine": 120
        }
    },
    "trailingWhitespaceOnLine": {
        "enabled": false,
        "config": {
            "numberOfSpacesAllowed": 1,
            "oneSpaceAllowedAfterOperator": true,
            "ignoreBlankLines": true
        }
    },
    "maxLinesInFile": {
        "enabled": false,
        "config": {
            "maxLinesInFile": 1000
        }
    },
    "trailingNewLineInFile": { "enabled": false },
    "noTabCharacters": { "enabled": true }
    }
}

[duckmatt]

I'm not 100% sure about the formatting rules, I think if we enable them by default it will cause a lot of noise - hopefully most projects are keeping their formatting somewhat consistent so if a rule is broken it's likely broken consistently throughout their project. In the case of the console app I think it's potentially acceptable as someone is choosing to run the tool, but when it comes combined with the IDE I think it will cause a lot of frustration, the likely outcome would be a lot of people disabling the linter (it's also not easy to disable all the formatting rules in one go). We could potentially have a default config for IDE, and another for console

One thing I'm also wondering with the formatting rules is if there's anyway we can delegate parts of it to Fantomas. My worry is we end up re-implementing parts of what they have, as the string based formatting checks may be quite ridden with edge cases. There's also the potential to end up with contradictory rules between the tools

[jgardella]

In my opinion I think we should take this opportunity to align the default rules as much with community opinion on correct style as possible; based on the current style guide most of the formatting rules would be part of that and enabled by default. I do see the concern with IDE users; is the main concern there with Ionide? I suppose some people are probably using Ionide without realising some of the suggestions are coming from FSharpLint. We could address this within Ionide/FsAutoComplete by passing in an explicit config to the call to the linter in the case when there is no fsharplint.json file, rather than falling back to whatever we have defined as the defaults in this repo. Basically I would prefer to update the defaults in this repo if we can, and we can handle backwards compatibility to avoid friction in this change in the FsAutoComplete/Ionide as we see fit (although ideally I would eventually want them converging on our defaults as well). Also, it is not that big of a deal in my opinion as users can add a config file with the old defaults if they want, and it would probably be a good thing for more users to be aware that the linter exists and they can configure how it works (although of course best to raise that awareness with minimal friction).

Regarding Fantomas, what exactly do you mean by "delegate parts"? Would we remove any rules that the linter has that Fantomas is already handling? I do think it is important to address how Fantomas and FSharpLint should work together as they are serving a similar purpose, and that is likely a bigger discussion. At the same time I would not want to remove functionality from the linter because it already exists in Fantomas... there may be users who want to use the linter but not Fantomas, or vice versa, and those users should ideally not have missing functionality as opposed to users of both tools.

[milbrandt]

At least the Community SonarQbe F# plugin is only using FSharpLint and not Fantomas, as it only scans and will not format on it's own. So I'm in great favour of keeping all rules here in FSharpLint.

[milbrandt]

IMHO following rules should be activated by default:

• trailingWhitespaceOnLine - editors can fix this on saving (no whitespaces at line end) and it reduces merge conflicts
• maxNumberOfBooleanOperatorsInCondition - the limit is difficult to define, but helps to keep the code maintainable
• nestedStatements - see maxNumberOfBooleanOperatorsInCondition for the reasons
• maxCharactersOnLine - in a former company the limit was 72 or in extraordinary cases 79 (C++, not FORTRAN). This is very restrictive, but I'm annoyed if I have to scroll horizontally to see all of the line.

[duckmatt]

By delegate, I mean making use of Fantomas in some way as a library, it has had years of good work put into it, if we do formatting it makes sense to leverage that. Their tool is more popular and already integrated into both Rider and Ionide, having duplicate rules in the two tools which could behave differently doesn't feel like a good direction to be heading in. Although having a separate formatter and linter is also nothing bad - not uncommon. Keeping the two and conflating them is potentially confusing, although I think using Fantomas as a library from the linter is a good middleground

I think if we're going to get IDE adoption with the linter enabled by default, then either the IDEs need their own config, or the formatting rules should be disabled by default, it'll be down to the IDE maintainers at the end of the day but I think unlikely they'll want formatting warnings by default. With Ionide we'll either need do as you say (pass in a custom config - although I think that config should be defined here e.g. IDE defaults config), or give them a heads up to see if the rules are going to work for them.

[duckmatt]

IMHO following rules should be activated by default:

trailingWhitespaceOnLine - editors can fix this on saving (no whitespaces at line end) and it reduces merge conflicts
maxNumberOfBooleanOperatorsInCondition - the limit is difficult to define, but helps to keep the code maintainable
nestedStatements - see maxNumberOfBooleanOperatorsInCondition for the reasons
maxCharactersOnLine - in a former company the limit was 72 or in extraordinary cases 79 (C++, not FORTRAN). This is very restrictive, but I'm annoyed if I have to scroll horizontally to see all of the line.

Agree on these, although maxCharactersOnLine by default in an IDE is potentially controversial. I'd be tempted for a minimum of 120 which seems to have taken over from 80

[milbrandt]

120 is perfectly fine for me - I know it's controversial and wanted to point out that there are still today reasons (company rules and maintainability, I know that some people say even set a limit of 200)

[jgardella]

By delegate, I mean making use of Fantomas in some way as a library, it has had years of good work put into it, if we do formatting it makes sense to leverage that.

I am definitely open to this, but would want to see what this implementation would look like; from a quick glance at the Fantomas API I'm not sure if it would be easy to achieve right now. Maybe @nojaf could provide some insight. In any case it will be good to have a discussion about how Fantomas and FSharpLint can work together to achieve their similar goals.

With Ionide we'll either need do as you say (pass in a custom config - although I think that config should be defined here e.g. IDE defaults config), or give them a heads up to see if the rules are going to work for them.

Yes, this makes sense, and I agree input from the IDE maintainers will be crucial. My hope was to get all tools aligned on the same defaults for consistency, and to try to help to establish a more standardized style for F#, but that is perhaps not a realistic goal within the v1 release; we can focus on achieving that within the FSharpLint repo and the dotnet tool for v1, and then work with users of the FSharpLint API to try to achieve more standardization in defaults moving forward. @Krzysztof-Cieslak do you have any input from the Ionide/FSAC standpoint?

[jgardella]

Agree on these, although maxCharactersOnLine by default in an IDE is potentially controversial. I'd be tempted for a minimum of 120 which seems to have taken over from 80

I agree on the controversy with having it on by default. For me it is something I keep disabled and keep max characters as more of a common sense/best judgement decision. And it may be difficult to come up with a setting that everyone would be happy with as a default. So I would prefer to have this disabled by default.

[Krzysztof-Cieslak]

My controversial opinion (and somehow related to the IDE part of the discussion) - things like maxCharactersOnLine or trailingWhitespaceOnLine shouldn't be part of FSharpLint at all. They can be handled by standard tools like EditorConfig that already has support in various (most) IDEs.

[jgardella]

@Krzysztof-Cieslak We welcome all opinions and would like to hear more of them, controversial or otherwise! :)

I see what you're saying about those rules being covered by EditorConfig. However I do still see value in having them in FSharpLint so that they can also be verified as part of a build pipeline by running the linter. EditorConfig and similar tools may be able to check these things, but we cannot force people to use them or to use IDEs that support them; having the rules in the linter allows users to enforce these rules and others as part of their build pipeline.

[nojaf]

Hello @jgardella, some input in regards to Fantomas:

• Settings are hard. Default settings are even harder.
  We've been using configuration files in Fantomas for 4 months now and it hard to say much about it. We don't really have any data of what settings people actually override, so it difficult to say if our defaults are good.
• We try to align to the F# style guide by MS for our settings but I'm not sure that is the way to go honestly. It is good to have such a thing but again usage: who is following that guide?
  -The guide is also not complete and has small issues. I open GitHub issues for that from time to time but with mixed successes.
• As for .editorconfig, yes everybody is a big fan until it comes to implementing them. It is possible to use Fantomas as CLI tool or via programmable API so if we want to support editorconfig we need to be able to parse this ourselves.
  And honestly, we could re-use two or three default settings so the JSON format was just a bit easier to work with.
• I also found didn't find much guidance for custom settings in editorconfig. Is it a "just do it" kind of thing?
• It would be great if FSharpLint and Fantomas could share some settings. Maybe this should even be a larger topic where the glossary for fsharp editor config items are defined.

I hope this was somewhat helpful, sorry for being a tad negative...

[jgardella]

Thanks for the info @nojaf. Regarding .editorconfig I agree there are a lot of questions there, and that is not something we are targeting supporting for the v1 release. I think the idea of sharing settings between FSharpLint and Fantomas would make sense; possibly we could have a separate project which parses F# rules from editorconfig (the glossary you mentioned), and then FSharpLint and Fantomas can implement whatever subset of those that they would like.

Also agreed on the MS F# style guide not being a very strong source of truth now. But I think we should strive to have some "definitive" F# style guide (which people can of course differ from as they see fit); the MS guide seems like an obvious starting point for that and maybe we can try to improve it.

Regarding the V1 release though, I would be most curious to hear your thoughts on the feasibility of F# utilizing the Fantomas API for some of the formatting-related rules we have defined. I understand @duckmatt's concern about having these defined in two different tools possibly leading to annoying inconsistencies. But I am also not sure if this is feasible given my understanding of the Fantomas API.

[nojaf]

Hey @jgardella, I had a change of heart when it comes to .editorconfig.
We've replaced our JSON config with .editorconfig.

Some details:

• When the setting is Fantomas specific, we prefixed it with fsharp_ (See default values)
• We now have a copy paste function in our online tool (see example)
  At the time of writing, this is still the JSON format. But the idea is to have .editorconfig format instead.
  This sorta makes up for the lack of intellisense.
• Editorconfig does allow you to set different settings for certain files or folders. Our previous config file was all or nothing.

If you are still considering using .editorconfig, please let me know.
Perhaps we can share some fsharp_ setting names.