Keep spaces for named arguments

[Viridovics]

Fixes #1877

[nojaf]

Hello @Viridovics, thank you for this first contribution to Fantomas.

There are a few things though, which you could not have known:

• As you are changing the style, this PR should be targeting the 4.7 branch.
  The reasoning behind this is that we can bugfix the 4.6 release on the master branch and process every new feature or stylistic change on the next minor. I really should document this, apologies.
• @dsyme and I spoke at a conference in Oslo, and there I vaguely remember some doubts about this part of the style guide. Although it is published, it was still a bit of a pending topic whether it should have been.
• Looking at the multiline example, it feels very weird to me to still keep the equals after the name

SomeFunction(
    name=
        SearchForName(
            "foooooooooooooooooooooooooooooooooooooooooooooooooo",
            "baaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaar"
        )
)

It just looks like name= in total is the identifier. Also, this multiline example isn't particularly mentioned in the style guide. It really should be in there as the next person is going to open an issue that Fantomas is doing something not specified by the guide.

• Taking a step back, in defence of keeping the space, we do put a space for record fields. Would it not make more sense to align these? Just a thought.

{ name =
    SearchForName(
        "foooooooooooooooooooooooooooooooooooooooooooooooooo",
        "baaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaar"
    ) }

• I would also like to hear the opinion of the G-Research style guide on this. Perhaps we would need a setting to control this.
  @Smaug123 and @kojo12228 would you mind bringing this up in the team?
• The current PR doesn't take patterns into account, if we are to have this, we should also include support for this:

let examineData x =
    match data with
    | OnePartData(part1=p1) -> p1
    | TwoPartData(part1=p1; part2=p2) -> p1 + p2

Excuses for all these open questions, I wholeheartedly understand this is might not feel like a welcoming experience for your first PR. Changing the style is just a very delicate course of action in Fantomas. Once this is merged, in my experience someone will challenge this greatly and be annoyed that the style changed again.

@dsyme, @Smaug123 and @kojo12228 could you respond to my questions above? Many thanks!

PS: @dsyme, out of interest, would there be any room to have a special named argument expression node in the AST?
The infix application with the equals sign feels more like a hack than an actual representation. Would this be ok to have? Would simplify Fantomas a bit.

[Smaug123]

The GR style guide says "Named arguments should also not have space surrounding the =", but this situation essentially never arises internally and I don't think we have a strong opinion. We'll ask the team, though.

[Smaug123]

Yeah, we don't have a strong opinion and will align with Microsoft, whatever you decide.

[nojaf]

Alright, thanks for confirming.

[Viridovics]

Hello @nojaf !

Thank you for detailed review!

Created list needed fixes:

• change destination branch to 4.7
• restore space for multiline function argument
• delete spaces for single line pair of NamePatPairs

[nojaf]

Hi @Viridovics, thanks for the additional changes for SynPat.

restore space for multiline function argument

We don't really have a consensus on this yet. I mentioned that I found the absence of a space weird in the multiline assignment but that doesn't mean we need to go with it. I would really like to hear @dsyme his thoughts on this.

In a perfect world, all of the stylistic debate would already have been done at fsharp/fslang-design. But as I mentioned, I recall some doubt on the topic.
So, we need to verify this first before we ship a change in Fantomas.
The ripple effect is quite huge when people would update to 4.7, every codebase using Fantomas would be affected.
Thanks for understanding.

[dsyme]

The current style guide says "no spaces", and all the code I personally write by hand has "no spaces" and so the current style guide reflects the visual expectations the language was originally designed for.

However when I started using Fantomas more I found myself thinking this may have been a relic of an overly cramped, compact coding style. Looking at the samples in the tests I still find myself thinking this.

@Smaug123 Do you really not have a strong preference?

[Smaug123]

@dsyme Maybe a very mild personal preference for having more spaces, but it really doesn't arise much on the code I'm used to (which strongly prefers a single record containing config, rather than named arguments).

[auduchinok]

@dsyme I find the no-space style too cramped and quite different from other similar places in the language where we do have spaces: we either have spaces on the both sides (binary operator expressions, tuple types, bindings, etc), or a space after the separator (tuple expressions and patterns, parameters, typed expressions), so I always put spaces for named arguments and patterns, and it looks kind of more inline with the rest of the language to me (but maybe it requires some getting used to).

Maybe we could have a setting here? At least, if we don't change the default to have spaces 😅

[nojaf]

I honestly think we should keep the space as it will be more consistent with the rest of the language.
Just this morning I wrote:

match x with
| SynExpr.TryFinally (trivia={ TryKeyword = mTry })  -> mTry

It is weird to explain to a newcomer, that there should be no space for trivia, but one inside the record.

[knocte]

I honestly think we should keep the space
It is weird to explain to a newcomer, that there should be no space for trivia, but one inside the record.

Completely agree with this.

[dsyme]

I honestly think we should keep the space

OK, given the collected feedback we will keep the spaces. Can someone submit a PR to change the style guide please?

Thanks
Don

[nojaf]

Can someone submit a PR to change the style guide please?

Yes, I can pick this up later this week.

@Viridovics I'm sorry this might have been an anticlimatic outcome for you. If you were rooting to remove the space in Fantomas, I understand this can be very frustrating.

I still think your changes are an improvement in CodePrinter.fs, we could keep those.
Since the style no longer changes, you can target the master branch again.

Thank you for understanding and thank you for your patience

[Viridovics]

Hello @nojaf !

I've changed destination branch to master. Should I change anything else in this PR? Thank you.

[nojaf]

Hi @Viridovics! Well, given the outcome of this discussion, the space should be present again.
So, this impacts any new tests you have written and you need to revert the existing tests.
Thanks!

[nojaf]

Oh, and you might want to rebase again the latest master.

[nojaf]

Hi @Viridovics, I'm sorry for this next request but as the style guide was updated, the normal flow of Fantomas will actually already lead to the desired outcome.
I tested this locally and reverting all changes to CodePrinter will lead to all tests passing (including the new ones).
So, in that case, I'm in favour of less is more. Could you revert the changes to CodePrinter, please?
Sorry, for all the hassle.

[Viridovics]

Hi @nojaf, no problem. I've reverted CodePrinter.fs. Keeped only new unit tests.