Skip to content
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

Review long function definitions #946

Closed
4 of 5 tasks
nojaf opened this issue Jul 3, 2020 · 6 comments · Fixed by #947
Closed
4 of 5 tasks

Review long function definitions #946

nojaf opened this issue Jul 3, 2020 · 6 comments · Fixed by #947

Comments

@nojaf
Copy link
Contributor

nojaf commented Jul 3, 2020

I propose that Fantomas formats long function definitions by default as described by the Microsoft style guide and place the GR style behind a setting.

The existing way of Fantomas deals with this problem is using the G-Research style guide.

Pros and Cons

The advantages of making this adjustment to Fantomas are that we respect both style guides.

The disadvantages of making this adjustment to Fantomas are an extra setting to maintain.

Examples

Please provide multiple examples (before and after scenarios) and explain what rules did apply to achieve the outcome.

Microsoft:

let fold (funcs: ResultFunc<'Input, 'Output, 'TError> seq)
         (input: 'Input)
         (input2: 'Input)
         (input3: 'Input)
         : Result<'Output list, 'TError list> =
    ()

G-Research

let fold
    (funcs: ResultFunc<'Input, 'Output, 'TError> seq)
    (input: 'Input)
    (input2: 'Input)
    (input3: 'Input)
    : Result<'Output list, 'TError list>
    =
    ()

Extra information

Estimated cost: S

Related suggestions: #935, #492, #769

Affidavit (please submit!)

Please tick this by placing a cross in the box:

Please tick all that apply:

  • This is not a breaking change to Fantomas
  • I or my company would be willing to help implement and/or test this
  • This suggestion is part of the Microsoft style guide (please add a link to section if so)
  • This suggestion is part of the G-Research style guide (please add a link to section if so)
@nojaf
Copy link
Contributor Author

nojaf commented Jul 3, 2020

@Smaug123 @JackMatusiewiczGR suggestions for a setting name are welcome.
I'm currently thinking about AlternateLongFunctionSignature as boolean.

@Smaug123
Copy link
Contributor

Smaug123 commented Jul 4, 2020

I think AlignFunctionSignatureToIndentation is more descriptive (as opposed to "align function signature to first parameter").

@auduchinok
Copy link
Contributor

auduchinok commented Jul 13, 2020

What about using a different (well, third already) approach: do double indent for signature continuation and then a single indent for the right-side block?

let internal UpdateStrongNaming (assembly: AssemblyDefinition)
        (key: StrongNameKeyPair option) =
    assembly.Name

I've initially seen this style in Kotlin and really like how it fits in various F# parts too, e.g:

match foo with
| Some x when
        someExpressionHere ||
        andAnotherOne ->
    printfn "%O" x
| _ -> ()

@nojaf
Copy link
Contributor Author

nojaf commented Jul 13, 2020

Hmm, I'm not entirely convinced here. I feel like the alignment of the parameters we currently have in the other styles is more readable, to be honest.
Maybe I'll reconsider in the future but I'd like to wrap up 4.0 so this is not something I want to wait for.

@auduchinok
Copy link
Contributor

Making it double-indent makes it consistent regardless of what was on the previous line, so in this particular case it doesn't matter how long the function name is, you'll see the next parameters at the same indent in any case (and most likely will have much more space in the line left).

@nojaf
Copy link
Contributor Author

nojaf commented Jul 15, 2020

it doesn't matter how long the function name

This is what the GR style guide proposes, regardless of the name the indent will be one in this case.
And the equal sign is on its own line so that makes it clear when the function body is starting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants