Fix SpaceBeforeArgument edge cases

[VelocityRa]

Fixes #554 and #555.

105 tests need adjustment, I suppose I should do that in a separate commit? They're a lot fewer if I change the default.

I looked at a bunch of them and most seem alright, but match patterns are changed too, ie. this:

fantomas/src/Fantomas.Tests/ComputationExpressionTests.fs

Line 62 in cf56bae

| Tree(x, left, right) ->

becomes

          | Tree (x, left, right) ->

Is that acceptable?

[VelocityRa]

No feedback received, so I pushed adjustments to the tests.

Some edge cases that don't work are constructor calls via new (ie. new WebClient()), calls to exceptions raised with raise (ie. raise (InnerError("inner")) and method chains (ie. WebHostBuilder().UseConfiguration(config)).

These look like separate bugs, since neither addSpaceBeforeParensInFunCall nor addSpaceBeforeParensInFunDef are called for them, so I think I'm going to leave them aside for this PR.

Not sure where to put tests for these, so I'm just going to make a new file.

[VelocityRa]

I tried

[<Test>]
let ``SpaceBeforeArgument option``() =
    formatSourceString false """
    let oneArg(foo: int): int = foo
    oneArg(2)
    """ config
    |> should equal """
    let oneArg (foo: int): int = foo
    oneArg (2)
    """

And I'm getting the following error with no more useful information:

Fantomas.FormatConfig+FormatException : Parsing failed with errors: [|/src.fsx (3,5)-(3,11) parse error Unexpected identifier in implementation file|]

Is that invalid F# or something? I'm new to the language.

[nojaf]

Hello @VelocityRa , to see if your F# code is valid you can always use the ast-viewer. If it doesn't parse there, you have a syntax error somewhere.

No feedback received

Don't take this the wrong way but it was hard to see the impact of the PR without running the tests locally. So I indeed deliberately waited until the build was green to see the outcome.

When looking at the changes to the tests I'm afraid I can't approve these as too many things changed. This will appear as breaking behaviour and people will be upset about this.

To move forward I propose to introduce a new set of smaller settings that control one thing only and replace SpaceBeforeArgument with these.
This way we can proceed in a way that does not break the existing tests (so please revert those).

• SpaceBeforeUnitParameterInLowercaseFunctionDefinition (default = false)

changes

let a() = 10

to

let a () = 10

when true

• SpaceBeforeUnitParameterInUppercaseFunctionDefinition (default = false)

changes

let A() = 10

type Person() =
    member this.Walk() = meh

to

let A () = 10

type Person () =
    member this.Walk () = meh

when true

• SpaceBeforeParenthesesParameterInLowercaseFunctionDefinition (default = true)

changes

let bar(a: int) = a

to

let bar (a: int) = a

when true

• SpaceBeforeParenthesesParameterInUppercaseFunctionDefinition (default = false)

changes

let Bar(a: int) = a

type Person(x: string) =
    member this.Walk(a:int, b:string) = meh

to

let Bar (a: int) = a
type Person (x: string) =
    member this.Walk (a:int, b:string) = meh

when true

• SpaceBeforeUnitArgumentInLowercaseFunctionCall (default = false)

changes

foo()

to

foo ()

when true

• SpaceBeforeUnitArgumentInUppercaseFunctionCall (default = false)

changes

Foo()
person.ToString()

to

Foo ()
person.ToString ()

when true

• SpaceBeforeParenthesesArgumentInLowercaseFunctionCall (default = true)

changes

bar(7 + 8)

to

bar (7 + 8)

when true

• SpaceBeforeParenthesesArgumentInUppercaseFunctionCall (default = false)

changes

Bar(8+8)

to

Bar (8+8)

with these new settings, I would keep the existing behaviour of SpaceBeforeArgument intact by applying the suggested defaults.

All these new settings would obviously need an extensive set of unit tests to make sure that we covered all scenarios.

Make sure to cover:

• function definitions
• Object method calls
• Lamba
• Signature files
• Pattern matches on discriminated unions
• Class/Struct definitions
• ... (things I can't come up with right now)

Not sure where to put tests for these, so I'm just going to make a new file.

You should add new tests in files where the code is related to the settings.
I think LetBindings, LongIdentWithDotsTests, SignatureTests, ClassTests, StructTests

[VelocityRa]

Thanks for the extensive feedback @nojaf.

to see if your F# code is valid you can always use the ast-viewer.

I did use https://repl.it/languages/fsharp to test it and it worked fine there, but in the fantomas test it still threw the error, so I assumed I was doing something else wrong. So, still not sure what happened there.

Don't take this the wrong way but it was hard to see the impact of the PR without running the tests locally. So I indeed deliberately waited until the build was green to see the outcome.

I see. I suspected that this might be the case, but the unfortunate consequence was that I had to waste at least an hour adjusting all these tests, knowing that it would likely get rejected - just to get some feedback.

In this case it was funded so I don't mind as much - but usually it won't be so here's some feedback for you too as fellow OSS maintainer: not a great way to welcome new contributors to your project.

---

As I mentioned, this work is funded and the agreed upon scope was much smaller than what's being requested.
If I change the default to false, I "only" need to adjust 37 tests instead of 105. That would not be acceptable either I imagine?

If not, and I implement all the options you asked for, would it be acceptable if the edge cases mentioned in #556 (comment) weren't fixed? I have not looked at these at all and they involve parts of the codebase I'm not familiar with.

And what about the tests? I'm really new to F# so it'd take a while time for me to come up with and write an extensive collection, so how necessary would they be for my work to be merged?

[knocte]

@nojaf when you say Uppercase or Lowercase I guess you mean PascalCase and camelCase, respectively.

But... don't get me wrong, are you seriously proposing 8 new settings? is this to deter us from making the contribution? Why not create a new SpaceBeforeArgumentStrict setting that covers all these cases, to avoid breaking backwards-compatibility but at the same time keep some sanity?

[nojaf]

Changing the default settings and slightly changing some tests is not an option.
Either we solve this styling problem thoroughly or we don't. I want a solution where there is a lot of flexibility so that it is future proof and easy to maintain. Each new setting is so small that it represents only one thing and cannot invoke any ambiguity. To summarize please take the high road on this.

@VelocityRa I will create a contribution guideline document in the coming weeks. There I will elaborate on why unit tests are so crucial to this project and provide a set of guidelines to set the correct expectations when creating a PR.

@knocte I used the word Lowercase instead of camelCase to avoid confusion when someone would write weird constructs like let foo_bar () = 42. The Lowercase setting should still work here even though the identifier is snake case.

[smoothdeveloper]

@nojaf, thanks for the review, I'd like to put minor notes for consideration:

For the sake of the codebase/implementation, I find it important to finely discriminate the usages of such settings (like the way you did), it doesn't mean all has to be surfaced yet as user visible settings; but it means the implementation adheres to a fine understanding of the formatting choices according to the language concepts, which I think is a good concern to have beside looking at particular issues individually.

If possible we should split the settings for function and member definition (+ constructors you mentionned in chat), would that make sense? probably remove the lower/upper case variants for definition sites unless such distinction remains useful on top of the split on the type of definition.

For the call sites, as is (based on untyped AST), using the StartsWith lower/upper case remains necessary.

Is there a motivation for Fantomas to evolve to also use the typed AST or there are already identified reasons (too complex, or others) to not go there eventually?

[smoothdeveloper]

I had to waste at least an hour adjusting all these tests

@VelocityRa the time spent on fixing the tests is not "wasted", it helps highlight all the cases that are impacted by the change to existing logic.

Not to sound too optimistic, but I'm sure people are going to be willing to help going with a fine adjustment of settings. Technically speaking your contribution address the funded issue, and initiates helpfull discussion for the project to keep moving in the right direction wrt to final implementation and elaborating more contributions guidelines, etc.

[nojaf]

@smoothdeveloper lower/upper case variants for definition sites would be necessary to preserve the current style. Formatting using the default settings in the next version of Fantomas should lead to same result. You do have a point though that the problem lowercase/uppercase is more one on the calling side of functions. So I'm still a bit on the fence on this one.

I'm on board to even further split up functions, member definitions and constructors. This might be a correct argument to drop lower/upper case distinction and keep those strictly for the calling side.

I have never had any motivation to look at TAST, gut feeling this would bring in a whole new level of complexity and I don't think it is necessary to solve these cases.

[knocte]

@knocte I used the word Lowercase instead of camelCase to avoid confusion when someone would write weird constructs like let foo_bar () = 42. The Lowercase setting should still work here even though the identifier is snake case.

Fair enough.

I'm on board to even further split up functions, member definitions and constructors. This might be a correct argument to drop lower/upper case distinction and keep those strictly for the calling side.

Would this translate in less settings? TBH I think the UX would be greatly diminished by having such an explosion of settings when typing --help for something that should really be very simple. I mean, myself I'm going to turn all this SpaceBeforeArgument subsettings to true for my project. Maybe wrap all of them under a single --spaceBeforeArgumentAlways flag?

[nojaf]

I guess we could wrap all settings into one flag from the command line perspective only.
The internal configuration record could still work agnostic from the combine setting.
However, I'm not sure how that would play out if you set --spaceBeforeArgumentAlways in combination with something else. A workaround could also be something like this.

Would this translate in fewer settings?
Probably not, it might end up around the same amount.

[VelocityRa]

What about

If not, and I implement all the options you asked for, would it be acceptable if the edge cases mentioned in #556 (comment) weren't fixed? I have not looked at these at all and they involve parts of the codebase I'm not familiar with.

[nojaf]

Please implement this in a way that doesn't require any modifications in the existing tests.
My approach listed in this thread should be of help.

[VelocityRa]

Got it; so it's fine if not all edge cases work, as long as no tests are changed.

[knocte]

so it's fine if not all edge cases work, as long as no tests are changed.

I don't interpret @nojaf's comment this way. What I get is that each edge case should be covered by a new setting (and a new setting deserves new tests, hence why all existing tests that run with default settings don't need changing). Am I right @nojaf ?

[VelocityRa]

If that's the case, then he still hasn't answered my question.

Above (#556 (comment)) he detailed 8 new options (which I already knew I will have to implement/test) and they are all irrelevant to the edge cases I've asked twice about, which will need no additional tests since in that scenario I would not be touching them at all.

[nojaf]

Yes @knocte that is what I mean.
So don't change any code in the existing tests and proceed by adding new settings to implement the behavior.

You need to see it like this: someone formats code with the current version of Fantomas and the default settings.
When that person formats the same code with the next version of Fantomas (with again the default settings), the result should be the same.

[VelocityRa]

Yes. I know. I got that part.

Please read #556 (comment). This is what I have asked about 3 times by this point.
Do you care about these or not.

[nojaf]

The new settings should have an impact on those things as well.

[nojaf]

Closing in favor of #649