requireSpaceBetweenArguments: new rule

[jamesallardice]

Validates that call expression arguments have the correct spacing before and after.

Closes #771

[coveralls]

Coverage increased (+0.07%) when pulling 82d5744 on jamesallardice:argument-space-rule into 32da403 on jscs-dev:master.

[indexzero]

How is this different than "validateParameterSeparator"

[indexzero]

Yeah, sorry didn't mean to be short there, just got to Paris. Super jet lagged.

This looks to be exactly the same rules though: https://github.com/jscs-dev/node-jscs/blob/master/README.md#validateparameterseparator

[jamesallardice]

@indexzero This rule validates the separation of arguments in call expressions. The validateParameterSeparators validates the separation of parameters in function signatures:

function a(b, c, d) {} // validateParameterSeparators
a(1, 2, 3); // requireSpaceBetweenArguments

See #771.

The code is largely similar between the two (only difference is the error message itself) but I couldn't see a way to DRY it easily (doesn't seem to be anywhere to put code relevant to multiple rules). Open to suggestions if I've overlooked something.

[indexzero]

Ah ok. Thanks for explaining it to me! Is the plan to move "validateParameterSeparators" to "requireSpaceBetweenParameters" in 2.0.0 then?

[jamesallardice]

No problem. Yeah I believe so. At least that's what I took this to mean:

we're trying to unify rule names and values.

Rule naming seems a lot more consistent that the last time I contributed which was a good few months ago. Most rules are now disallow-* or require-*.

[mrjoelkemp]

Should this be a valid separator? It's not in the list of acceptable values for this rule.

[jamesallardice]

@mrjoelkemp I think it probably should be. I'm sure there are are people who write that way. I can remove it if you think it's not very useful or I can add it to the readme.

[mrjoelkemp]

If this is a "valid" separator, your rule's regex might need to be strengthened to disallow that.

[mrjoelkemp]

I think it's useful, but the rule is meant to force a space between arguments. Forcing ',' (no spaces) is best for disallowSpaceBetweenArguments

[jamesallardice]

@mrjoelkemp Sorry, to clarify that, it is a valid separator. It just isn't mentioned in the list of acceptable values. I will add it to the list.

[mrjoelkemp]

Great job on this @jamesallardice! It's looking good except for a the ',' false positive. It seems like we also need the new rule: disallowSpaceBetweenArguments. I imagine that's simpler than the rule you implemented; would you mind giving that a go?

[jamesallardice]

@mrjoelkemp Sorry, GitHub failed to notify me about half of your comments! Yes, I agree, I will remove the ',' option and add a separate rule for disallowing spaces.

[coveralls]

Coverage increased (+0.07%) when pulling d001e96 on jamesallardice:argument-space-rule into 32da403 on jscs-dev:master.

[markelog]

How about true value, it should solve

doStuff(a, b, c); // valid
doStuff(a,b,c); // invalid

Whereas value would be more consistent with other rules, yes, it would be not flexible as it could have been, but really, who would write code like:

doStuff(a , b ,);

?

That would mean implementing opposite rule too.

[jamesallardice]

@markelog I think that's a good idea. I based this rule off the validateParameterSeparator rule which is why it has the values it does. Since we've already established that we'll need to opposite rule I think it would be good to just have a boolean value... I guess the question is does anyone write code like this:

doStuff(a , b , c);  // I doubt it
doStuff(a ,b ,c);    // Maybe

So I also guess it depends on how flexible we want to be. Allow people to define their own separators, or just cater for a, b, c and a,b,c.

[markelog]

a, b, c and a,b,c

I think those cases could be used in the presets, when others are not, so they are probably more common then others.

I would say, let's be a little less flexible and a little bit more consistent.

[jamesallardice]

I think the solution now follows what we discussed. There are 2 rules, requireSpaceBetweenArguments and disallowSpaceBetweenArguments. This allows validation of the two common cases, a(b, c) and a(b,c).

Should the commits be squashed so it appears as it was done this way first time around or do you prefer to keep the history?

[coveralls]

Coverage increased (+1.0%) when pulling 329f06e on jamesallardice:argument-space-rule into d931b4f on jscs-dev:master.

[markelog]

Should the commits be squashed so it appears as it was done this way first time around or do you prefer to keep the history?

Took care of it, thank you!