fsprojects / FSharpLint

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

New linter is going crazy on perfectly fine elmish code #296

Open forki opened 5 years ago

forki commented 5 years ago

Originally reported to ionide. Please see https://github.com/ionide/ionide-vscode-fsharp/issues/997

jgardella commented 5 years ago

Hi @forki.

This rule is also based on the F# style guide, which suggests always using parentheses around tuple instantiations. As @MangelMaxime suggested in the linked issue, it can be disabled through the config.

However if I'm understanding your comments on the linked issue correctly, it sounds like this is a common format in elmish/SAFE stack code? I was not aware of this. Given that this is in the official style guide, I think it makes sense to have it as a default. If elmish/SAFE code differs from the style guide in this instance or others, maybe we could create a config file specifically for elmish/SAFE projects?

forki commented 5 years ago

Actually this is so established right now, that I think the "official style guide" needs to be changed. Adding parens here breaks basically all the historical code in light syntax and adds a lot of clutter. /cc @cartermp

forki commented 5 years ago

I opened up a pr on the styleguide. https://github.com/dotnet/docs/pull/10186

I don't want to be a dick here and will happily accept community decision.ut this one feels so completely anti-F# for someone like me who uses it like first inofficial code drops.

jgardella commented 5 years ago

I think it's a great discussion to have, thanks for starting it. Personally, I like parentheses around tuple instantiation (and even around tuple pattern matching) as I think it makes the code easier to read. But again I have not used elmish/SAFE so I may not be seeing the full picture. Let's continue the discussion over on your style guide PR.

MangelMaxime commented 5 years ago

Considering the changes requested by @forki on the guidelines. If this is accepted, we could add a mode "strict" to FSharpLint for people who want to always put parens around their tuples.

The idea being because it's mentioned as optional people could want to force them self to always use parens even for function return.

forki commented 5 years ago

Why would anyone ever do that? It's a light language mode - next thing we do is require people to put semicolons at the end of each line ;-)

Am Mo., 28. Jan. 2019, 22:16 hat Maxime Mangel notifications@github.com geschrieben:

Considering the changes requested by @forki https://github.com/forki on the guidelines. If this is accepted, we could add a mode "strict" to FSharpLint for people who want to always put parens around their tuples.

The idea being because it's mentioned as optional people could want to force them self to always use parens even for function return.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fsprojects/FSharpLint/issues/296#issuecomment-458304387, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgNAt1jpnCGTyyg4TswbYcdG5V_LYvks5vH2iWgaJpZM4aWYpI .

MangelMaxime commented 5 years ago

No idea, but just because we don't know doesn't me but can't provide an option to support it if some want to and because the code is already written for it. ^^

I believe in configuration against strict walls. Even, we good defaults a tool doesn't all suits the need for everyone.

forki commented 5 years ago

Yes config is good, but if we manage to settle on good default it's even better. I'm willing to do compromises.

Am Mo., 28. Jan. 2019, 22:53 hat Maxime Mangel notifications@github.com geschrieben:

No idea, but just because we don't know doesn't me but can't provide an option to support it if some want to and because the code is already written for it. ^^

I believe in configuration against strict walls. Even, we good defaults a tool doesn't all suits the need for everyone.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fsprojects/FSharpLint/issues/296#issuecomment-458316623, or mute the thread https://github.com/notifications/unsubscribe-auth/AADgNGhIny0y-B9Hh_psoRmyS-Yv6bThks5vH3FogaJpZM4aWYpI .

cartermp commented 5 years ago

I think having something like a requirement for all tuples to be surrounded by parentheses could make a good setting, but the default should be to allow them at the return site like the docs suggest in light of elmish styles.

forki commented 5 years ago

Cool, so the suggestion to open up the restriction is merged to the styleguide.

jgardella commented 5 years ago

Great, happy we were able to reach a decision and update the guide. I'm thinking for FSharpLint we can make this setting configurable with the following options:

MangelMaxime commented 5 years ago

For me having both of these options make sense indeed.