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

Improve formatting #501

Closed
cmeeren opened this issue Oct 8, 2019 · 13 comments · Fixed by #1145 or #1148
Closed

Improve formatting #501

cmeeren opened this issue Oct 8, 2019 · 13 comments · Fixed by #1145 or #1148

Comments

@cmeeren
Copy link
Contributor

cmeeren commented Oct 8, 2019

(Sorry for the non-descriptive title)

Description

The following is hand-crafted, and I find it aesthetically pleasing and tidy:

module Program

open Microsoft.AspNetCore.Hosting
open Microsoft.Extensions.Hosting
open Serilog
open Startup

[<EntryPoint>]
let main args =
  Host
    .CreateDefaultBuilder(args)
    .ConfigureWebHostDefaults(fun builder ->
      builder
        .CaptureStartupErrors(true)
        .UseSerilog(dispose = true)
        .UseStartup<Startup>()
      |> ignore
    )
    .Build()
    .Run()
  0

Here is how Fantomas formats it, which I find rather messy:

module Program

open Microsoft.AspNetCore.Hosting
open Microsoft.Extensions.Hosting
open Serilog
open Startup

[<EntryPoint>]
let main args =
  Host.CreateDefaultBuilder(args)
      .ConfigureWebHostDefaults(fun builder ->
      builder.CaptureStartupErrors(true).UseSerilog(dispose = true).UseStartup<Startup>
        () |> ignore).Build().Run()
  0

There are several issues here:

  • I'm not fond of the (initial) fluent calls to Host being aligned the way they are. If Host had a much longer name, all the interesting code would be pushed far to the right, leaving little space and yielding many unneeded line breaks.
  • The body of ConfigureWebHostDefaults is indented on the same level as the line above. It should be indented an additional level.
  • There's a weird line break after UseStartup<Startup> with the unit arg being on the next line
  • .Build() and .Run() should be on the same level as the other fluent calls to Host

Repro code

Online (on my PC the line width it set to 120 when clicking the link; I made it with 80, so that's probably another bug)

@nojaf
Copy link
Contributor

nojaf commented Jul 15, 2020

The thing I'm missing somewhat here is when not to stack them on multiple lines.
For example:

let foo = Host.CreateDefaultBuilder(args)

As this is short, I'd leave this as it is.
First options that come to mind here are:

  • extra setting to control from what length it should start the multi line approach.
  • Or do respect the MaxWidth of the page here to determine this?

@cmeeren
Copy link
Contributor Author

cmeeren commented Jul 15, 2020

An extra setting would be the most flexible solution, similar to e.g. MaxInfixOperatorExpression as I understand it. If it's below that, use a single line. If it's above, use multiple lines.

@nojaf
Copy link
Contributor

nojaf commented Jul 17, 2020

How about MaxChainedExpressionWidth?

@cmeeren
Copy link
Contributor Author

cmeeren commented Jul 17, 2020

Sure, that works. Could also consider a name containing Fluent. But perhaps that's too specific? Your suggestion sounds fine to me.

@quintusm
Copy link

The lack of this functionality is my only major issue with Fantomas as it stands today. It would be great if we could have something implemented for this.
Personally I could live with an "always stack chained calls in column" in setting, even if it leads to splitting short lines such as

let foo = Host
             .CreateDefaultBuilder(args)

Thank you very much for all the great work being done....

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 28, 2020

I agree with @quintusm; this (as well as #988) is what prevents me from actually using Fantomas. After these are fixed, I'll probably be able to format all my codebases using Fantomas. Before that, the Fantomas formatting would require enough manual tweaks for me that I'd rather wait.

@nojaf
Copy link
Contributor

nojaf commented Sep 1, 2020

Hey @quintusm and @cmeeren, thanks again for expressing these missing features.
I can't really say when I will get to those but reminders are good nonetheless.
In #420 I've implemented a feature to ignore certain files when formatting, maybe this can be of use until I get to the other stuff.

@nojaf
Copy link
Contributor

nojaf commented Sep 18, 2020

I've pushed the first alpha with support for this issue at https://www.nuget.org/packages/fantomas-tool/4.2.0-alpha-001.
See docs.
Please try this out and provide feedback.
Thanks.

@cmeeren
Copy link
Contributor Author

cmeeren commented Sep 18, 2020

Looks good for the most part, but this fails:

Original and expected:

let firstName =
    define
        .Attribute
        .ParsedRes(FirstName.value, FirstName.create)
        .Get(fun u -> u.FirstName)
        .SetRes(userSetter User.setFirstName)

Actual (notice .Attribute.ParsedRes on one line):

let firstName =
    define
        .Attribute.ParsedRes(FirstName.value, FirstName.create)
        .Get(fun u -> u.FirstName)
        .SetRes(userSetter User.setFirstName)

Compiles, but the formatting is incorrect. IMHO it should chop either no lines or all lines, to preserve the symmetry.

@nojaf
Copy link
Contributor

nojaf commented Sep 19, 2020

Published alpha-002 that tackles this example.
Please reopen if something is still not quite ok.

@cmeeren
Copy link
Contributor Author

cmeeren commented Sep 19, 2020

I can't reopen, but something is still not right. Function bodies are not always indented correctly:

Original and expected:

let getColl =
  define
    .Operation
    .ForContext(Context.toAuthenticatedContext)
    .GetCollection(fun _ parser ->
      let x = 2
      x)

Actual:

let getColl =
  define
    .Operation
    .ForContext(Context.toAuthenticatedContext)
    .GetCollection(fun _ parser ->
    let x = 2
    x)

By the way, you may consider indenting function bodies either relative to . or relative to the character following .. Up to you; I have no strong opinions on the matter.

@nojaf
Copy link
Contributor

nojaf commented Sep 21, 2020

Published alpha 003, let me know if I need to reopen this.

@cmeeren
Copy link
Contributor Author

cmeeren commented Sep 22, 2020

Looks good, thank you so much!

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