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

Option for Stroustrup bracket style #1408

Closed
7 of 11 tasks
josh-degraw opened this issue Jan 28, 2021 · 28 comments
Closed
7 of 11 tasks

Option for Stroustrup bracket style #1408

josh-degraw opened this issue Jan 28, 2021 · 28 comments

Comments

@josh-degraw
Copy link
Contributor

josh-degraw commented Jan 28, 2021

I propose we...
Add the ability to use Stroustrup bracket styling.

The existing way of Fantomas deals with this problem is ...
Use the existing bracket formatting styles, which are not commonly used in most mainstream programming languages.

Pros and Cons

The advantages of making this adjustment to Fantomas are ...
Allowing people to have more control over how they want to format their code, and having a more widely-used (esp. outside of F#) formatting style as an option.

The disadvantages of making this adjustment to Fantomas are ...
Adding another configuration option,technical complexity, and potential conflicts with existing formatting rules.

Examples

The officially recommended way to declare the following record type is as follows:

type PostalAddress =
    { Address: string
      City: string
      Zip: string }

For many, this style feels strange. My team and I prefer to use Stroustrup style e.g.

type PostalAddress = {
    Address: string
    City: string
    Zip: string
}

We like this because, among other reasons, it reduces the used horizontal space a little, makes it easier to reorder members, and just feels less messy. I am wanting to integrate fantomas to keep our code more consistent, and wanted to see if we could utilize that style for formatting at least record types.

I have used the fsharp_multiline_block_brackets_on_same_column=true that feels a little better:

type PostalAddress =
    {
        Address : string
        City : string
        Zip : string
    }

But it would be awesome to be able to format our code how we actually want it.

Extra information

Estimated cost (XS, S, M, L, XL, XXL):
L

Related suggestions: (put links to related suggestions here)

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)

For readers:

Please leave:

  • 👍if you use Fantomas today and would enable this setting
  • 🎉if you do not use Fantomas today and would start using it after this feature
  • 👎if you use Fantomas today and would not enable this setting.

Remaining work

@nojaf
Copy link
Contributor

nojaf commented Jan 30, 2021

Hello Josh, thank you for taking the time to write this elaborated request.
This, unfortunately, is exactly what we don't want in this project.

@nojaf nojaf closed this as completed Jan 30, 2021
@nojaf
Copy link
Contributor

nojaf commented Dec 23, 2021

Hello @josh-degraw, I'm having a change of heart when it comes to this request.
This is a complex subject, so I'll try to elaborate a bit on my motivations for re-opening this issue.

First, this issue has been requested multiple times in various shapes and forms.
Some people proposed it slightly different or on other platforms/repositories but they all are more or less after the same thing:

someExpression openingDelimiter
    multilineBodyExpression
closingDelimiter

From a technical point of view, it is quite challenging to get right and implement in the codebase.
It happens for a certain set of scenarios (which are open for interpretation) and there is no easy fix to capture them all.
That has for me been a good reason not to have this at all.

On the other hand, I get this request multiple times a year, for many years now.
I understand that people write code like this today and struggle to adapt to what Fantomas outputs.
It even serves a bit as a marker to see who is using Fantomas and who is not.
There is a lot of code in this style out there and I do recognise this fact.

Recently, I was inspired by some unrelated events and started working on a prototype for this.
You can see some progress in my somewhat secret ragnarok branch.
I got a chunk of it working and it is interesting to see where this goes.
The technical impact is what I expected and I do find some puzzling edge cases.

Besides the code part, another thing that I'm struggling with is determining the scope of this setting.
You listed some examples, but there is more to it from an AST point of view.
I'd like to bring a compelling and consistent story that works good enough for the community.
Not everybody's wishlist will be a part of the scope, but hopefully, it still might hold some value for most.
We need something sensible and determine where the boundaries lie.
Some things will make the cut, and some won't.
This should be documented as an architecture decision record.

Lastly, as this is not part of any style guide, I also don't want this setting to be a precedent that every request can make it into Fantomas.
Style should be discussed in the style guide.
Future style requests will most likely still be closed on sight if they have no backing by the style guide.
And yes, that will be a very unfair action considering that we might ship this current feature.

From a timeline point of view, this won't go overnight.
This will be a 4.7 feature and development in 4.6 still needs to wrap up.
Poke me on Gitter if you have any questions there.

All that said, I'm looking for help from the community.
Who is interested in contributing to this feature?

Happy to hear all your thoughts,

Florian

PS: tagging some people in no particular order:
@dsyme, @baronfel, @jindraivanek, @Smaug123, @stackedsax, @jgiannuzzi, @c-rindi, @auduchinok, @l3m, @ForNeVeR

@dsyme
Copy link
Contributor

dsyme commented Dec 23, 2021

"somewhat secret ragnarok branch" - it's worth contributing just for that :)))

@baronfel
Copy link
Contributor

The main tension I see here is between the project goals of:

  • increasing adoption of the Fantomas tool in general,
  • having a consistent set of defaults to use as a metric for what code style/formatting options should exist in Fantomas (in this case the F# Style Guide being the source of truth)

I 100% agree that this change should be done in the name of reducing barriers for using the tool for projects and ecosystems that don't want to abide by the F# Style Guide for whatever reason. My main worry is that without a firm enough set of guidelines for what is allowable in this new mode, the definition of the mode will be shaky and be more of a grab-bag of 'styles that XYZ people like' instead of a consistent design rule.

@JordanMarr
Copy link

I wouldn't do it to gain new users considering all the ongoing effort that would need to be poured into handling this with the new edge cases it would introduce. Going out of your way to take on more complexity for something that is outside of the official style guide seems like a bad move for you.

I can easily imagine someone voting "I would start using Fantomas if I could format my code this way" only to run into a new edge case bug and then quickly uninstalling it. 🤷‍♂️

@KathleenDollard
Copy link

A style is a set of decisions. One set is laid out in the Style Guide.

I understand @baronfel 's concern over "a grab bag of 'styles that XYZ people like', but I think it's worth exploring whether this request represents an alternate style guide. Any set of design decisions also needs to make sense together. Is this a single feature or a slightly different way to look at code? If the second, then this is almost by definition just the most obvious of a set of issues that would have to all be resolved to make Fantomas usable in that style.

I like the commitment Fantomas has to the Style Guide because it allows most F# to look the same and the value to reading code is significant. I like that this issue has a lot of friction to get it done, and the commitment that it isn't opening the door to a ton of other features, committing to very high friction for any future requests. The Style Guide support (and support for it in major editors) is what makes Fantomas Fantomas.

That said, if the community can help build this, it seems there is enough push (people not feeling they can use Fantomas), and the complexity can be reduced in design or otherwise made reasonable it seems OK to do.

@josh-degraw
Copy link
Contributor Author

@nojaf I appreciate the additional look into this and willingness to potentially include it! I'd love to help contribute however I'm able.

@cartermp
Copy link

cartermp commented Dec 23, 2021

So I'm not inherently opposed to this, but I will note that the F# style guide currently states that these aren't okay, but rather the allman-style is: https://docs.microsoft.com/en-us/dotnet/fsharp/style-guide/formatting#formatting-record-declarations

From a technical standpoint it is the most flexible option, since there's no requirement to use with ... end to attach members to the record declaration.

That could change in the compiler of course, but as it stands, it's what I personally use for anything that isn't a 1-liner.

@twop
Copy link

twop commented Dec 23, 2021

PLEASE, that will be a game changer for me. I have to use

{
 a = 1
} 

all the time, because I found myself often adding or shuffling fields around which is extremely painful to do with the official style guide formatting (I use "copy line" shortcut that captures either { or } which I have to remove afterwards). With Stroustrup formatting it would be so much better!

@nojaf
Copy link
Contributor

nojaf commented Dec 24, 2021

Hello all,

I'll try to address some comments below, wonderful to see this interaction.

@baronfel right on the money, enough guidelines will be required to indicate the scope of this setting. I don't think one simple rule will cover it, it will more or less be a set of rules when it applies and when it doesn't.

@JordanMarr you are correct as well, the folks that don't use Fantomas easily give up when one (little or large) thing isn't working as they hoped it would. Of all the people reacting 🎉, it is no guarantee they will become users after this setting.

@KathleenDollard I'm not sure this should be brought into the broader scope of an alternate style guide. It could just be documented as a way of dealing with a number of things instead of how the entire F# language specs should look like.
A bit similar to how the Elmish setting came to be. I'd like to avoid another style guide for the time being as that has the potential of introducing yet another sleeve of settings.
I'm more a fan of focusing on a good F# style guide where the vast majority of the community can get behind.

@josh-degraw many thanks for still being interested in contributing to this setting. I propose we organize some sort of kick-off meeting early next year.

@cartermp, you are correct that this setting would require with ... end in order to support members. Perhaps, to minimize the impact we can limit the scope of record type definitions to only allow Stroustrup formatting when there are no members/interfaces involved.

type V = {
    X : int
    Y : int
}

type W =
    {
        X : int
        Y : int
    }
    member this.Foo () = ...

Exceptions like this might be a necessary evil to keep the impact on the codebase to a minimum.

@olivercoad
Copy link

@cartermp can you explain the purpose for the style guide saying these aren't ok or point to some discussions about it? If there's not already been a a definitive decision, it might be worth opening an issue to add this style to the official style guide.

The best I could find is this, which feels a little underwhelming now considering the impact.

Also, I always dislike it when fantomas moves things to put the closing bracket on the same line as the last item, but it's worth mentioning that in some cases it does this even when the style guide doesn't.

@KathleenDollard
Copy link

@nojaf

My comment about an alternate style guide was unclear: I can't tell if this is a true one-off thing, or the tip of an alternate style guide iceberg. I do not think an alternate style guide, or the C# combine random features to create your own style, would benefit F#.

@dbrattli
Copy link

dbrattli commented Jan 4, 2022

I vote 👍 since it would enable me to write F# that looks more like Python (PEP8). The two styles allowed in Python are:

mydict = {
    'key': 'value',
    'key': 'value',
    ...
    }

OR

mydict = {
    'key': 'value',
    'key': 'value',
    ...
}

The styles will require the use of with for attaching members, so I think we should allow if with is used.

@akhansari
Copy link

Personally, I prefer to see a single style absolutely everywhere.
Like gofmt with no configuration file.
It doesn't matter if it's one style or the other.

gofmt's style is no one's favourite, yet gofmt is everyone's favourite.

@aloisdg
Copy link

aloisdg commented Jan 14, 2022

@akhansari or haskell's fmt, frontend's prettier, python's black, or the young csharpier. Like you I am all for "one true way". More options means more time spend to talk about them.

@TheAngryByrd
Copy link
Contributor

Would this apply to lists/arrays as well? I find myself struggling with adding to lists using the current style, I find the elmish style much better but I'm unsure when fantomas chooses to use elmish style.

For a concrete example, adding tests to Expecto feels awkward as I have to go searching for the single ] and put my cursor in between that and the last test. For example:

let tests =
        testList
            "samples"
            [ testCase "Add two integers"
              <| fun _ ->
                  let subject = Say.add 1 2
                  Expect.equal subject 3 "Addition works"
              testCase "Say nothing"
              <| fun _ ->
                  let subject = Say.nothing ()
                  Expect.equal subject () "Not an absolute unit"
              testCase "Say hello all"
              <| fun _ ->
                  let person =
                      { Name = "Jean-Luc Picard"
                        FavoriteNumber = 4
                        FavoriteColor = Red
                        DateOfBirth = DateTimeOffset.Parse("July 13, 2305") }

                  let subject = Say.helloPerson person

                  Expect.equal
                      subject
                      "Hello Jean-Luc Picard. You were born on 2305/07/13 and your favorite number is 4. You like Red."
                      "You didn't say hello" ]

I think I would much prefer the elmish style applied to this so that it looks more like as it feels much more ergonomic to add elements to a list.

let tests =
        testList "samples" [ 
            testCase "Add two integers"
            <| fun _ ->
                let subject = Say.add 1 2
                Expect.equal subject 3 "Addition works"
            testCase "Say nothing"
            <| fun _ ->
                let subject = Say.nothing ()
                Expect.equal subject () "Not an absolute unit"
            testCase "Say hello all"
            <| fun _ ->
                let person =
                    { Name = "Jean-Luc Picard"
                    FavoriteNumber = 4
                    FavoriteColor = Red
                    DateOfBirth = DateTimeOffset.Parse("July 13, 2305") }

                let subject = Say.helloPerson person

                Expect.equal
                    subject
                    "Hello Jean-Luc Picard. You were born on 2305/07/13 and your favorite number is 4. You like Red."
                    "You didn't say hello" 
        ]

I know this is similar but different, if this is something to discuss on fsharp-lang instead let me know.

@nojaf
Copy link
Contributor

nojaf commented Mar 4, 2022

Well, to some extent, that is the million-dollar question: what is included in this setting and what not?
Better expect support has been requested before in #1275.

The elmish pattern currently captures one identifier followed by one or two lists.

foo [] [
   // content
]

Expecto, in this case, adds an additional string into the function application.
As this deviates from the Elmish active pattern, it will format according to the default principles.

It would be possible to extend that active pattern I think. However, it might be interesting to investigate the options in regards to this setting.

@TheAngryByrd
Copy link
Contributor

TheAngryByrd commented Mar 4, 2022

Given this answer, it feels like it's outside the scope of this exact issue. Sounds like something I should argue for on the style guide. Thanks!

@olivercoad
Copy link

Would this apply to lists/arrays as well?

I had assumed it would. Assuming this setting does apply to lists, it seems like it would fall directly in the scope of this issue. Why would it matter if the list is part of a function call?

@Shuenhoy
Copy link

Shuenhoy commented Mar 5, 2022

For the lists/arrays issue, I think a slightly more flexible solution would be being conservative with adding or removing newlines. For example, the default TypeScript formatter of VSCode has such behavior. It seems to always keep the user's original line breaks.
Although, I am not sure how hard to implement this behavior in the current codebase of fantomas.

@nojaf
Copy link
Contributor

nojaf commented Mar 5, 2022

I had assumed it would.

Please don't assume anything at all here. There is no clear line in this story in regards to what is covered and what is not.
The original description says nothing about arrays or lists.

Why would it matter if the list is part of a function call?

The is extremely relevant in terms of implementation. Every case where this setting should apply needs to be spelt out here, to the letter in technical terms. And in this case, it is not a single construct, is it a combination of two (or more) syntax tree nodes.
I tried to capture this process in this document, to elaborate on what analysis really needs to be done here. That document is not at all complete.

Just assuming list/arrays are part of this, is not going to cut it, at all. Please help discover the technical boundaries of this setting and verify that they are even possible to implement in the codebase. There is a crystal clear reason why we as maintainers don't want this setting, so the only way we can have something is by an elaborate technical proposal.

I think a slightly more flexible solution would be being conservative with adding or removing newlines.

We tried that once in the past, and it is very hard to implement correctly. We ended up removing the setting because it only really worked from time to time. I'm not saying we can never revisit this, but it goes against the philosophy. Fantomas currently does not preserve any stylistic information. If we were to keep this kind of information, the code would be formatted differently when developer A wrote it versus developer B. Currently, after Fantomas was applied, you can no longer tell who originally wrote the code, and that is the beauty of it.

@Shuenhoy
Copy link

Shuenhoy commented Mar 6, 2022

We tried that once in the past, and it is very hard to implement correctly.

I understand this may be hard in the current codebase, and not asking it to be implemented any sooner.

Currently, after Fantomas was applied, you can no longer tell who originally wrote the code

This would be great if the formatter can produce satisfying results in most of the situations. However, I think that is even more difficult and would introduce more and more exceptions and special cases like elmish, Expecto... On the other hand, keeping users' line breaks would be a workaround and get rid of all these problems. Even go-fmt, which is known to be tough that doesn't have any stylistic options, behaves in such a way.

@nojaf
Copy link
Contributor

nojaf commented Mar 6, 2022

On the other hand, keeping users' line breaks would be a workaround and get rid of all these problems

This is really is an oversimplification of a layered problem. Even if it were technically possible, you are missing the point of this project. The output is compliant with the style guide. Keeping users' line breaks won't always be compatible with the recommended practice.

Back on topic, perhaps Elmish/Expecto can fit into this setting.
They all share that they are function applications that end with an array/list.

So in theory, this setting could have the following logic:

  1. Print everything on one line if possible
  2. Print the function name and remaining (but last) parameters on the same line.
  3. Add the opening bracket after the penultimate argument
  4. Indent and add a new line
  5. Print the contents of the last argument collection on each line
  6. Unindent and add a new line
  7. Print the closing bracket.
// short
functionName arg1 [ listArg1; listArg2 ]

// long
functionName arg1 [
    listArg1
    listArg2
]

This has a similar ring to the rules of placing lambda's at the end of a function application

In case the first line is longer than max_line_lenght, all arguments should be placed underneath each other.

functionName
    arg1 
    [
        listArg1
        listArg2
    ]

In case, there are two lists as arguments, perhaps something like could be suitable:

functionName [
    list1Arg1
    list1Arg2
] [
    list2Arg1
    list2Arg2
]

It is a slight variation on what we do today but without vanity alignments.

@nojaf
Copy link
Contributor

nojaf commented Apr 2, 2022

Hi all, I've updated the original issue with some topics that will need to be addressed. The list is probably still somewhat incomplete, I'm merely trying to move forward with this.
Happy to hear your feedback. Please speak up if you want to contribute to anything.

@natalie-o-perret
Copy link

natalie-o-perret commented Oct 19, 2022

Hi all, I've updated the original issue with some topics that will need to be addressed. The list is probably still somewhat incomplete, I'm merely trying to move forward with this. Happy to hear your feedback. Please speak up if you want to contribute to anything.

Bumped into this issue and others as I was trying to set up something sensible in our repo, and found myself banging my head against the wall since I leverage a lot the keyword of the CE on the same line as the let binding:
fsharp/fslang-design#706 (comment)

re pasting here the example in the other issue of the style guide:
e.g.

let doStuffAsync myParameter = async {
    let! someOtherThing = giveMeSomeOtherThingAsync()
    return myParameter + someOtherThing 
}

I've found myself a bit lost, not sure if that stance is somehow of a heresy but I kinda consider [| and [ to be just like any other CE.

@josh-degraw
Copy link
Contributor Author

josh-degraw commented Oct 20, 2022

I mentioned in the fslang-design issue as well, but I think whether or not to put computation expressions in their own line potentially deserve their own formatting setting, independently of Stroustrup. The idea behind this setting is around brackets, while a CE is an expression. The brackets in CEs almost always use the "Stroustrup" style anyway independently of this setting, so I think it merits its own issue/discussion.

@ly29
Copy link
Contributor

ly29 commented Nov 12, 2022

Just want to mention that this change have made fantomas acceptable within my team and we are now applying to project after project.

@nojaf
Copy link
Contributor

nojaf commented Apr 19, 2023

I'm going to close this issue as this is now part of the style guide and available in v6.

Any further expansion on this topic should follow the existing process of the style guide.

Last but not least, I would like to thank @josh-degraw for stepping up and contributing the majority of this feature.

@nojaf nojaf closed this as completed Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests