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

issue #550 - Allow non-indented "break" in "switch" statement #568

Closed
wants to merge 2 commits into from

Conversation

kefniark
Copy link
Contributor

I just modified validate-indentation.js to allow non-indented "break" in a case statement.
I also modified and add new unit tests to match those changes.

List of proposal :

  1. Modified validate-indentation to permit initialization with an object (as validate-jsdoc or validate-line-breaks). Number and String initilization still works fine, it's just more flexible this way
  2. Added a new option "allowBreakUnindented" with a default value : false
  3. Added a new option "allowCaseUnindented" with a default value : true
  4. Added unit tests to be sure that validate-indentation still works accurately

So, this rule default behavior doesn't change at all.
New functionnalities are just options that you can enable or not.

with allowBreakUnindented = false (default)

switch(value){
    case '1':
        break; // ok
    case '2':
    break; // error
}

with allowBreakUnindented = true (fix #550 issue)

switch(value){
    case '1':
        break; // ok
    case '2':
    break; // ok
}

And 'allowCaseUnindented' parameter is the same thing for 'case' indentation in the switch statement.

For initialization, you can still use :

  • Initialization with a string :
    validateIndentation: '\t'
  • Initialization with an integer :
    validateIndentation: 4

But you can also use an object with optional parameters :
validateIndentation: { indentChar: '\t', indentSize: 2, allowBreakUnindented: true}

@mikesherov
Copy link
Contributor

Thanks for contributing! One of the design approaches for validateIndentation is that it should require almost no configuration. The current validateIndentation, for example, allows both indented case vs. switch or not, depending on the style of the first case statement.

You should be able to do a similar technique with case vs. break. Can you please explore that before introducing new options?

@kefniark
Copy link
Contributor Author

Ok, it seems more easier this way. So I will remove those parameters and let it work as you said for initialization.

I just have one question about "You should be able to do a similar technique with case vs. break".
For case element, the style depend on the first case element, then the following code doesn't work :

switch(value){
case '1':
    break;
    case '2':
    break;
}

For "break", do I need to consider the same things and have the same style for all "break" in the same switch statement ?

The following example is OK

switch(value){
    case '1':
    break;
    case '2':
    break;
}

But this one should not work

switch(value){
    case '1':
    break;
    case '2':
        break;
}

@mikesherov
Copy link
Contributor

For "break", do I need to consider the same things and have the same style for all "break" in the same switch statement ?

Yes. Thanks for tackling this!

@kefniark
Copy link
Contributor Author

I think this should be OK

@ronkorving
Copy link
Contributor

So how do I enforce the following?

switch (foo) {
case 'bar':
  break;
case 'bob':
  break;
}

(Crockford style)

@kefniark
Copy link
Contributor Author

Hi Ronkorving,

By default, crockford style indentation is working fine (even before this issue).
This is what mikesherov said earlier by

The current validateIndentation, for example, allows both indented case vs. switch or not, depending on the style of the first case statement.

If you're looking for a way to be more strict, only accept this indentation style and refuse all others. Unfortunately there is no way to do this for the moment.
Even with using --preset crockford, indentation checking will still be the same.
A strict mode (with parameters) could easily added to the validateIndentation.js but this is another issue.

@mdevils mdevils force-pushed the master branch 2 times, most recently from db9f580 to 2fa147c Compare August 27, 2014 00:48
@sylouuu
Copy link

sylouuu commented Sep 10, 2014

Hi, something new here?

Bests

@kefniark
Copy link
Contributor Author

In #550 @mikesherov said 3 weeks ago :

I plan on land a bunch of stuff tomorrow, if @markelog doesn't get to it first.

but after that nothing changed, this issue is still a "new rule value" and the #550 is still open.
This should be available soon

@mikesherov
Copy link
Contributor

@kefniark thanks for contributing. Occasionally, I have other priorities that get in the way of developing and maintaining free software.

As soon as I find some time, this will be reviewed and merged, just like the dozens of releases that came before it.

@mikesherov
Copy link
Contributor

@kefniark reviewing now. Thanks again for the patience.

var loc = node.loc;
if (indents === 0) {
linesToCheck[loc.end.line - 1].popAfter = false;
if (linesToCheck[loc.end.line - 2].push !== 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

save linesToCheck[loc.end.line - 2] off into a variable please.

@mikesherov
Copy link
Contributor

@kefniark ping!

@kefniark
Copy link
Contributor Author

@mikesherov pong!
Thanks for this review, I'm sorry I don't have time to fix it now.
But I'll try to take care of it within 1 or 2 days

@mikesherov
Copy link
Contributor

No worries. Whenever you can... we all get busy ;-)

@kefniark
Copy link
Contributor Author

Ok, so I fixed most importants problems that you have noticed :

  • early returns
  • space between 'function' and '('

there a second iteration of SwitchStatement

I replaced it with a BreakStatement iteration which doesn't need any foreach loop and which is more simple. I don't know why I directly did it like this last month :/

@mikesherov
Copy link
Contributor

@kefniark, can you please rebase this pull request? Also, have you run npm test? I'm fairly certain there's still a few lint errors left.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.72%) when pulling a07d00a on kefniark:master into 2fa147c on jscs-dev:master.

@mikesherov
Copy link
Contributor

@kefniark I don't know what you did, but whatever it is isn't right.

@kefniark
Copy link
Contributor Author

Yes I know, I tried to use a tool provided by phpstorm which should have rebase my forked repo. It seems that he have merged all the update since the fork.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) when pulling a38c4c3 on kefniark:master into 467a170 on jscs-dev:master.

@@ -45,6 +48,21 @@ module.exports.prototype = {
return firstContent;
}

function markBreakPop(node, indents) {
var loc = node.loc;
if (indents !== 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

move this line up.

Copy link
Contributor

Choose a reason for hiding this comment

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

meaning, move the entire if above the var declaration.

@mikesherov
Copy link
Contributor

@kefniark thanks again! Once these minor style nits are addressed, this should be good to go!

@mikesherov mikesherov closed this in bd62246 Oct 8, 2014
@mikesherov
Copy link
Contributor

@kefniark I ended up rewriting this patch quite a bit after removing the popAfter property. You still get credit for having done the work, but please take a look on what I settled on in bd62246 . Thanks again!

@sylouuu
Copy link

sylouuu commented Oct 15, 2014

Hi,

Just updated my gulp-jscs and I have the latest version:

npm ls | grep jscs
├─┬ gulp-jscs@1.1.2
│ ├─┬ jscs@1.7.1
│ │ ├── esprima-harmony-jscs@1.1.0-dev-harmony

I had "validateIndentation": 4 in my .jscsrc.

I tried:

"validateIndentation": {
    "indentSize": 4,
    "allowBreakUnindented": true
},

or even:

"validateIndentation": {
    "indentChar": "\t",
    "indentSize": 4,
    "allowBreakUnindented": true
},

But I have a syntax error. How to use this feature?

Bests

@hzoo
Copy link
Member

hzoo commented Oct 15, 2014

@sylouuu try just

{
    validateIndentation: 4
}

@sylouuu
Copy link

sylouuu commented Oct 15, 2014

That was my old config, and I have the error for break indentation.

Error: Expected indentation of 20 characters at file.js :
  1069 |
  1070 |                    this.props.plugin_status = type;
  1071 |                break;
----------------------------^

@mikesherov
Copy link
Contributor

@sylouuu, JSCS looks at the first break statement it encounters in your code, and uses that as the standard for the rest of your code. Do you have an indented break anywhere in your code?

@mikesherov
Copy link
Contributor

@sylouuu, thanks again for contributing. Can you please file an issue rather than comment on a PR so if there is a bug, others will see it?

@sylouuu
Copy link

sylouuu commented Oct 15, 2014

@mikesherov Done #689

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.

Allow non-indented "break" in "switch" statement
7 participants