requireMultipleVarDecl: add 'skipWithEmptyLine' option #215

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

twoRoger commented Jan 27, 2014

No description provided.

Contributor

Famlam commented Jan 27, 2014

I wouldn't call it 'all' or 'strict mode'.
Strict mode would suggest to be the implementation of onevar of JSHint, which would also trigger for

function main() {
  var a=5;
  a += a;
  var b=5;
}

which is unfortunately not implemented (hmm, perhaps I should point to this in #102 ).
Instead, I would keep the current behavior as 'true' (and thus allow either true or "skipWithEmptyLine" after your commit).

Owner

markelog commented Jan 29, 2014

Contributor

mikesherov commented Jan 29, 2014

@twoRoger, thanks for contributing!

@markelog, I didn't write this sniff, but here's my opinion:

There's no need for an option here. This sniff was just initially written to be to brittle. Rather than check for other variable declarations and positioning, this just needs to check that each variableDeclaration has at most one variableDeclarator.

The test is good, but we should remove the option and rewrite the sniff as I described.

Thoughts?

@twoRoger, can you make that update?

Contributor

twoRoger commented Jan 29, 2014

@mikesherov Seems like the proposed update is related to "disallowMultipleVarDecl" rule only, not to the "require" one. Btw, "disallowMultipleVarDecl" rule is already implemented exactly as you described)

Contributor

mikesherov commented Jan 29, 2014

Yeah, you're right. Sorry! For this rule, we should check all elements of the parent collection for other variableDeclarations, not just the siblings.

Still no need for a new option. 

Right?

Mike

On Wed, Jan 29, 2014 at 5:40 AM, Igor Agarlev notifications@github.com
wrote:

@mikesherov Seems like the proposed update is related to "disallowMultipleVarDecl" rule only, not to the "require" one. Btw, "disallowMultipleVarDecl" rule is already implemented exactly as you described)

Reply to this email directly or view it on GitHub:
mdevils#215 (comment)

Contributor

mikesherov commented Jan 29, 2014

Not just the immediate siblings, I meant. 

Mike

On Wed, Jan 29, 2014 at 6:49 AM, Mike Sherov mike.sherov@gmail.com
wrote:

Yeah, you're right. Sorry! For this rule, we should check all elements of the parent collection for other variableDeclarations, not just the siblings.
Still no need for a new option. 
Right?
Mike
On Wed, Jan 29, 2014 at 5:40 AM, Igor Agarlev notifications@github.com
wrote:

@mikesherov Seems like the proposed update is related to "disallowMultipleVarDecl" rule only, not to the "require" one. Btw, "disallowMultipleVarDecl" rule is already implemented exactly as you described)

Reply to this email directly or view it on GitHub:
mdevils#215 (comment)

Contributor

twoRoger commented Jan 29, 2014

The current implementation is exactly what I need. In our team we allow multiple var statements per function, but consecutive var statements should be joined.

The following example is valid regarding to our codestyle:

function main() {
  var a = 1;
  a += a;
  var b = 2;
  return a + b;
}

Invalid example:

function main() {
  var a = 1;
  var b = 2;  
  a += a;
  return a + b;
}

The only thing I wanted to add is a relaxing option ("skipWithEmptyLine"), which would allow the following:

function main() {
  var a = 1;

  var b = 2;  
  a += a;
  return a + b;
}

Probably, here should be one more rule which would be an exact replacement for onevar from JSHint.

Contributor

mikesherov commented Jan 29, 2014

The opposite should be true. There should be an additional enforcing rule
that requires var statements to all be at the top (regardless of
whitespace). Note that the opposite rule doesn't have the "at the top" nor
"consecutive" rules, so those should be additional enforcing rules rather
than the default. JSCS has a principal of least assumptions rather than
most, and a principle of enforce the least per rule possible, so style
guides are easily composable.

Thoughts?

On Wed, Jan 29, 2014 at 8:26 AM, Igor Agarlev notifications@github.comwrote:

The current implementation is exactly what I need. In our team we allow
multiple var statements per function, but consecutive var statements
should be joined.

The following example is valid regarding to our codestyle:

function main() {
var a = 1;
a += a;
var b = 2;
return a + b;}

Invalid example:

function main() {
var a = 1;
var b = 2;
a += a;
return a + b;}

The only thing I wanted to add is a relaxing option ("skipWithEmptyLine"),
which would allow the following:

function main() {
var a = 1;

var b = 2;
a += a;
return a + b;}

Probably, here should be one more rule which would be an exact replacement
for onevar from JSHint.

Reply to this email directly or view it on GitHubhttps://github.com/mdevils/node-jscs/pull/215#issuecomment-33583897
.

Mike Sherov

Contributor

Famlam commented Jan 29, 2014

[if I understand you correctly]
In that case I would split it up like this:
requireMultipleVarDecl:

  • true: behaves like 'skipWithEmptyLine'
  • "strict": behaves like the current behavior

requireVarDeclOnTop:

  • true: requires all var declarations on top (of the current function, in case of nested functions)
    (And the combinations of true for the latter and "strict" for requireMultipleVarDecl would equal onevar)
Contributor

mikesherov commented Jan 29, 2014

Actually, onevar does not require the single var declaration to appear on
top...

requireMultipleVarDecl:
true: checks that the var declaration is the ONLY variableDeclaration in
it's parentCollection (so, neither skipWithEmptyLine nor the current
behavior).
"consecutive": ensure all variable declarations are in a row without
checking whitespace
"strict": consecutive plus no empty lines allowed

requireVarDeclOnTop:
true: checks that the first statement of a function is a var declaration
(which you can then combine with requireMultipleVarDecl to ensure one var
at the top, or many var at the top). A caveat for this rule should be to
allow for redefinition of parameters before var declaration.

On Wed, Jan 29, 2014 at 2:28 PM, Famlam notifications@github.com wrote:

[if I understand you correctly]
In that case I would split it up like this:
requireMultipleVarDecl:

  • true: behaves like 'skipWithEmptyLine'
  • "strict": behaves like the current behavior

requireVarDeclOnTop:

  • true: requires all var declarations on top (of the current function,
    in case of nested functions) (And the combinations of true for the
    latter and "strict" for requireMultipleVarDecl would equal onevar)

Reply to this email directly or view it on GitHubhttps://github.com/mdevils/node-jscs/pull/215#issuecomment-33620468
.

Mike Sherov

Contributor

Famlam commented Jan 29, 2014

Ah, right about onevar.
(as a sidenote: honestly, the name requireMultipleVarDecl is confusing; I had to read your 'true...behavior' line at least three times, with the name of the option in my head...)
With consecutive you mean the proposed skipWithEmptyLine option I think? I like your suggestion :)

Contributor

mikesherov commented Jan 29, 2014

yes, consecutive should allow empty lines... strict should disallow. I
agree about the name confusion!

On Wed, Jan 29, 2014 at 3:07 PM, Famlam notifications@github.com wrote:

Ah, right about onevar.
(as a sidenote: honestly, the name requireMultipleVarDecl is confusing; I
had to read your 'true...behavior' line at least three times, with the name
of the option in my head...)
With consecutive you mean the proposed skipWithEmptyLine option I think?
I like your suggestion :)

Reply to this email directly or view it on GitHubhttps://github.com/mdevils/node-jscs/pull/215#issuecomment-33624932
.

Mike Sherov

Contributor

mikesherov commented Feb 19, 2014

@Famlam, any update on this?

Contributor

Famlam commented Feb 19, 2014

@twoRoger you mean? It's not my branch, is it?

Contributor

mikesherov commented Feb 19, 2014

Apologies. I meant @twoRoger

Contributor

twoRoger commented Feb 19, 2014

Sorry, I'm not going to implement suggested behavior.
Feel free to close this PR :)

Contributor

mikesherov commented Mar 20, 2014

Closing this PR for now, I'll pick up on the decided behavior later. @twoRoger, thanks again for contributing!

@mikesherov mikesherov closed this Mar 20, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment