-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Revise F# formatting guide #25972
Revise F# formatting guide #25972
Conversation
@nojaf Could you review? This addresses https://github.com/dotnet/docs/issues/25917#issuecomment-913516706 but also rearranges the sections in the guide to follow a logical order. Please check these in particular
|
This was a bit hard to review, so I may have missed some things. As for the blank lines above comments, if the original code already had them Fantomas will do the right thing. |
@nojaf I'm going through the entire revised doc using the Fantomas preview online tools to check the formatting. Could you check this one? I was expecting Target.create "Build" (fun ctx ->
// code
// here
()) |
@nojaf Also this one, which formats the |
@nojaf There's also a discrepency with respect to named arguments - the style guide says "no space" |
@nojaf Here's another case of interest - records with XML doc comments on the fields. Looks a bit odd to place that brace on the last line |
@nojaf If you could check the above cases quickly that would be cool - if any of them are Fantomas bugs I can open issues. I think maybe some are the changes we've discussed either not yet implemented or not in preview tools |
Hey @dsyme, you are correct some stuff is not in the online tool yet (nor in a stable version). Target.create "Build" (fun ctx ->
// code
// here
()) is still open in fsprojects/fantomas#1858. let function1 arg1 arg2 arg3 arg4 = arg1 + arg2 + arg3 + arg4 + arg2 + arg3 + arg4 + arg2 + arg3 + arg4 Fantomas currently treats almost all infix operators the same. There is nothing in place for arithmetic operators and in general infix operators are quite challenging to get right. It is a bit on an ongoing dragon in the codebase. I'm open to some good heuristics there. let rainbow2 =
{ rainbow with
Boss = "Jeffrey"
Lackeys = [ "Zippy"; "George"; "Bungle" ] } Created fsprojects/fantomas#1876 let makeStreamReader x = new System.IO.StreamReader(path=x) we just format the shape of the argument AST here as is. Paren
(App
(NonAtomic, false,
App
(NonAtomic, true, Ident op_Equality, Ident path,
tmp.fsx (1,52--1,57) IsSynthetic=false), Ident x,
tmp.fsx (1,52--1,58) IsSynthetic=false),
tmp.fsx (1,51--1,52) IsSynthetic=false,
Some tmp.fsx (1,58--1,59) IsSynthetic=false,
tmp.fsx (1,51--1,59) IsSynthetic=false),
tmp.fsx (1,25--1,59) IsSynthetic=false), we should do something different when it is in the context of SynExpr.New. // Declaring additional members on PostalAddress
type PostalAddress =
{
/// The address
Address: string
/// The city
City: string
/// The zip code
Zip: string
} to preserve the original style here, you are looking for the fsharp_multiline_block_brackets_on_same_column setting perhaps? |
@nojaf Regarding records declarations (with/without XML doc comments):
This is tricky. Here's my thinking
Anyway, on summary on revising the guide I kind of feel that record definitions should get special treatment to support a nice look when XML doc commented. But I'm aware that this is a special case. Would be interested in your thoughts. |
Hmm, a bit tricky indeed. Although as I understand it there is a clear rule when to always use the wide record formatting:
This is a relatively easy thing to check in the syntax tree. Slightly related I've raised fsprojects/fantomas#1878. |
@nojaf Added fsprojects/fantomas#1879 where we can discuss |
I've updated the tool to use our |
Fantastic! @luisquintanilla @KathleenDollard could I have a review/approve when your ready? Thanks. |
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.
Thanks all for making these updates. Looks great. Just a few minor formatting issues 😉.
Can you also update the ms.date
value in the metadata to reflect this document has been recently updated.
@luisquintanilla This is ready, thank you!!
Thanks, all done
Updated |
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.
Looks good to me. Ready for merge.
@luisquintanilla Thanks! |
Summary
///
documentation commentsPreview Link