-
-
Notifications
You must be signed in to change notification settings - Fork 192
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
Feature: space between function and unit #649
Feature: space between function and unit #649
Conversation
src/Fantomas/FormatConfig.fs
Outdated
SpaceBeforeUnitParameterInUppercaseClassConstructor : bool | ||
SpaceBeforeUnitParameterInLowercaseClassConstructor : bool | ||
SpaceBeforeParenthesesParameterInUppercaseClassConstructor : bool | ||
SpaceBeforeParenthesesParameterInLowercaseClassConstructor : bool |
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.
why use the word "parameter" in some places and the word "argument" in others?
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.
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.
interesting 🤔
src/Fantomas/FormatConfig.fs
Outdated
SpaceBeforeUnitParameterInUppercaseClassConstructor = false | ||
SpaceBeforeUnitParameterInLowercaseClassConstructor = false | ||
SpaceBeforeParenthesesParameterInUppercaseClassConstructor = false | ||
SpaceBeforeParenthesesParameterInLowercaseClassConstructor = false |
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.
tbh I still think this explosion of config settings is a bit ludicrous, if I were you I would just have 3: SpaceBeforeFunctionCall, SpaceBeforeFunctionDefinition, SpaceBeforeClassConstructor
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.
All these settings are necessary because Fantomas now has a weird way of dealing with this.
In order to not break the existing behaviour, this fine-grained configuration is necessary.
Simply said if you format with the current version of Fantomas and later with a version containing this PR you want exactly the same output. Otherwise, this is a breaking change that will upset people.
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.
In order to not break the existing behaviour, this fine-grained configuration is necessary.
I get that, but my 2cents assume this catch. My view on this is: the past behaviour was wrong, and there are not many cases out there that would be affected by it, so it should be fine correcting this inconsistency. But hey I'm no maintainer, your call to make this unnecessarily complex because of some paranoid fear ;)
@@ -270,7 +491,7 @@ A default configuration file would look like | |||
"SpaceAroundDelimiter":true , | |||
"KeepNewlineAfter":false, | |||
"MaxIfThenElseShortWidth":40, | |||
"StrictMode":false | |||
"StrictMode":false |
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.
unneeded trailing whitespace addition?
Putting a note that with I'm not sure if this warant adjusting the new parameter names, maybe |
docs/Documentation.md
Outdated
"SpaceBeforeUnitParameterInUppercaseClassConstructor":false, | ||
"SpaceBeforeUnitParameterInLowercaseClassConstructor":false, | ||
"SpaceBeforeParenthesesParameterInUppercaseClassConstructor":false, | ||
"SpaceBeforeParenthesesParameterInLowercaseClassConstructor":false, |
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.
2 remarks:
- When the config setting says "functions" does it also mean methods? (just asking because I remember you opened a github issue in some MS docs repo where someone was advocating for defaults that differed depending if the invokation was on a function or a method)
- When the config setting says "class constructor" does it also mean the invokation of 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.
NOTE: you reduced ctor setting to one (SpaceBeforeClassConstructor) but here in the documentation there are still 4 ^
In general I'm pretty well in favour of this. Config settings are cheap. G-Research coding standards here are essentially "use a space wherever possible, but sometimes it's not possible, so don't use a space then". Things like |
…hub.com/nojaf/fantomas into feature/space-between-function-and-unit
@jindraivanek I reduced space before class constructor to one setting as it is something you previously had no control over anyway. As @Smaug123 said, settings are cheap so I don't see the issue in this one-time setup. |
@jindraivanek current proposal after confirming the major version: SpaceBeforeParameter = true
SpaceBeforeLowercaseInvocation = true
SpaceBeforeUppercaseInvocation = false
SpaceBeforeClassConstructor = false
SpaceBeforeMember = false I'll update the documentation in another PR as it should be revisited as a whole. |
Good to see the list of options being trimmed down! |
Hey @knocte, I am just reading this PR and I see some interesting technical opinion that you wrote, but wrapped up with patronising adjectives like "ludicrous", "paranoid", and "ridiculous". It's counterproductive. In order to help make this PR better, and Fantomas in general, I would advocate for focusing on the technical matters, and keeping belittling out of it. |
Sorry about that @jgiannuzzi |
Well, this is purely an esthetic choice, both @jindraivanek and I came to F# from a C# background. And there it just feels weird to write As @Smaug123 mentioned |
In this case I was talking about sane defaults, not explosion of settings ;) |
I also come from a C# background, and the coding guidelines I started with actually tell you to do this: https://www.mono-project.com/community/contributing/coding-guidelines/ |
@knocte I don't see any example on https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/inside-a-program/coding-conventions where they put a space before Not sure what why the Mono went a different route, but I hope we can wrap up the endless discussion about the settings... |
And in that guideline I don't see any example of where they do different things depending if the function being invoked is lowercase or not. I hope my point is clear enough now, because by the looks of the last comments, it's not ;) |
This PR splits
SpaceBeforeArgument
into a more fine-grained set of settings:See documentation.
The default settings did not change any unit test behaviour.
For the command line client, I'm forcing the user to use a configuration file.
Something we might want to do for all styling preferences.
Related discussion can be found in #556, #554 and #555.
These new settings allow the code to be formatted the first rule of the G-Research style guide, "Put a space between a function and ()".
Attached the binaries of PR, in case anymore wants to try this out.
space-before.zip
I'm currently not sure all cases are covered, having some doubt regarding class definitions.
Pinging people who might be interested @Smaug123, @saul, @knocte