Skip to content
This repository has been archived by the owner on Mar 23, 2024. It is now read-only.

disallowMultipleVarDecl: add exception for undefined variable declaratio... #749

Closed
wants to merge 1 commit into from

Conversation

hzoo
Copy link
Member

@hzoo hzoo commented Nov 5, 2014

...ns

Rather messy as boolean logic could be simpler?
Not sure about the option name exceptUndefined either.
#451

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 555e869 on hzoo:add-except-undefined into 4b6eca0 on jscs-dev:master.

var hasDefinedVariables = false;
var isForStatement = node.parentNode.type === 'ForStatement';

for (var i = 0, l = node.declarations.length; i < l; i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might also be able to do:

hasDefinedVariables = node.declarations.some(function(declaration) {
  return !!declaration.init;
});

@mrjoelkemp
Copy link
Member

Thanks for this @hzoo! Great work getting this together. I agree that the condition is hard to follow. See my suggestion for making it a bit more readable.

I'm pretty excited to land this, as it's one step closer to the grunt preset and JSCS being used throughout the Grunt plugin ecosystem.

@hzoo
Copy link
Member Author

hzoo commented Nov 7, 2014

K added your suggestions: removed 2 extraneous tests, cleaned up hasDefinedVariables var, and also the boolean condition

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 08eb39f on hzoo:add-except-undefined into 52dcc50 on jscs-dev:master.

@@ -1045,7 +1045,7 @@ Disallows multiple `var` declaration (except for-loop).

Type: `Boolean` or `String`

Values: `true` or 'strict' (to disallow multiple variable declarations within a for loop)
Values: `true` or 'strict' (to disallow multiple variable declarations within a for loop) or 'exceptUndefined' (to allow declarations where all variables are not defined)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please turn the options into a list now that there are 3 options each with a sentence explaining them

@hzoo
Copy link
Member Author

hzoo commented Nov 7, 2014

Should I be making changes in multiple commits or just force push the same one again?

@mrjoelkemp
Copy link
Member

Doesn't matter much; we'll clean up the commit messages before merging to master. Amend + force push is totally fine.

@hzoo
Copy link
Member Author

hzoo commented Nov 7, 2014

I guess it's just because of the outdated comments since looking at what changed again might be weird but ok cool.

@mrjoelkemp mrjoelkemp mentioned this pull request Nov 7, 2014
1 task
@hzoo
Copy link
Member Author

hzoo commented Nov 7, 2014

added list format to readme, reorder comments in https://github.com/jscs-dev/node-jscs/pull/749/files#diff-b6e5523d8d2184da7a33a74a9162fc95R34

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling e272664 on hzoo:add-except-undefined into 199c2ff on jscs-dev:master.

@mrjoelkemp mrjoelkemp closed this in d270ac2 Nov 7, 2014
@mrjoelkemp
Copy link
Member

@hzoo Great work! Thanks for contributing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants