Space before argument option is too general

[ovatsus]

When set to false, we get this:

x.ToLower()
Seq.map(fun x -> x * 2)
Console.Writeline("something")
Some(a, b) -> ()

But when it's false we get this:

x.ToLower ()
Seq.map (fun x -> x * 2)
Console.Writeline ("something")
| Some (a, b) -> ()

What I would like to get is this:

x.ToLower()
Seq.map (fun x -> x * 2)
Console.Writeline("something")
| Some (a, b) -> ()

My suggestion is to remove the option and:

1. If it's the unit () argument, don't add a space before the parens
2. If it's a function with curried arguments like Seq.map etc., add a space before the parens
3. If it's a function with tupled arguments, don't add a space before the parens
4. If it's a DU constructor, add the space before the parens

What do you think?

[dungpa]

Unfortunately, at the syntax level, Seq.map, Console.WriteLine and Some are all the same (they're just identifiers). So we can't implement your suggestion without understanding meaning of the code.

Regarding curried and tuple arguments, we don't know a function is curried until it has two arguments (where we already add spaces as needed). I would like to avoid the complication of type checking because we are able format a larger range of programs (including those syntactically correct but not compilable yet).

I think SpaceBeforeArgument is a reasonable tradeoff. Several people have said that they like the option, so I don't think it's that bad.

One possible heuristic is

• space after lower case function application (List.map and foobar)
• space after all pattern discriminators regardless of case (Some in a pattern)
• space after all single-name upper case function application (Some in an expression)
• no space for multi-name upper case function application (x.ToLower and Console.WriteLine)

since that correlates pretty closely to curried functions v. object-oriented methods etc.

[ovatsus]

If we don't have enough information, could we at least try to have some heuristics on this? The current behaviour is very unsatisfactory. What about this:

• If it's (), never add spaces
• if it's camelCase, it's probably an F# function, add a space
• if it's PascalCase, it's probably a method, don't add spaces

It would yield this:

x.ToLower()
Seq.map (fun x -> x * 2)
Console.Writeline("something")
| Some(a, b) -> ()

@dsyme, what's you're take on this?

[ovatsus]

Oh, you beat me to it :) Your heuristics are more complete than mine. Do you think we need any option at all for this? I would say not

Well, I think the above should be the default.

Having stricter options is probably useful - there may be some unusual applications of F# where consistency is needed regardless of capitalization (e.g. DSL applications where PascalCase and UPPERCASE and lowerCase carry different semantic meanings than normal .NET/F# methods)

[dungpa]

Ok, Don's heuristics are complete enough to implement.

@ovatsus What should we name the new option so that

by default it gives

x.ToLower()
Seq.map (fun x -> x * 2)
Console.Writeline("something")
| Some (a, b) -> ()

and

x.ToLower()
Seq.map(fun x -> x * 2)
Console.Writeline("something")
| Some(a, b) -> ()

otherwise?

[ovatsus]

"Never add spaces before argument", with default to off? Actually we shouldn't just call it argument, as that's misleading, if there's no parentheses we always have to put a space

[dungpa]

It's a good name. Changing argument to parenthesis doesn't work either since we only do so in function applications and patterns.

[ovatsus]

This rule "space after all single-name upper case function application (Some in an expression)" is tricky as it causes stuff like this:

Regex (regex).Match(str)

So I iterated until I got the least amount of unusual formatting in the tests, and ended up implementing these rules:

• Fluent style function chaining -> no spaces
• Implicit and explicit constructors -> no space
• Function application with () -> no spaces
• Function application with uppercase single-name or multiname identifiers -> no space
• All other function applications -> use setting
• Patterns with () (includes function definition) -> no spaces
• Patterns with uppercase single-name or multiname identifiers (includes function definition) -> no spaces
• All other patterns -> use setting

So this means that instead of

| Some (a) -> ()

we get

| Some(a) -> ()

But overall I think globally it's better than it was before. If we implement #69 we'll then get this:

| Some a -> ()

Alleviating the problem

[dungpa]

I admit it looks much better. Will review the pull request later today.

[ovatsus]

BTW, if you look at ovatsus@442a316, you'll see that I changed the casing of the tests because his is not processed right:

member x.foo (...)
override x.foo (...)
default x.foo (...)

Because at the point the decision is made, only "foo" and "(...)" is available. If we can pass forward the information stating that this is a class member, we can improve on this, but it was a big change, so i left that for later. It's usually only a problem when you have lowercase private members, which is not that common, you usually use let bindings for that

Can you publish an update to the VS binding? Would love to try things out after recent updates.

[ovatsus]

I couldn't install VS2012 SDK (probably related to having either VS2013 or Win8.1 installed), so what I'm doing to test inside Visual Studio is to replace the dlls directly under %LocalAppData%\Microsoft\VisualStudio\\11.0\Extensions\jo4nr22t.qpz
(Probably that jo4nr22t.qpz folder will change from machine to machine, but it's easy to find it)

[dungpa]

You can try work-in-progress version at https://dl.dropboxusercontent.com/u/15386265/Fantomas.VisualStudio.vsix

Note that issue #73 is still occuring.