Option for switch case indentation #1228

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
@WolfgangKluge
Contributor

WolfgangKluge commented Aug 9, 2013

Fixes #935: Adds an option for indentation of case/default-keywords

Option for switch case indentation
Fixes #935: Adds an option for indentation of case/default-keywords
@tlvince

This comment has been minimized.

Show comment Hide comment
@tlvince

tlvince Aug 15, 2013

👍

tlvince commented Aug 15, 2013

👍

@Mithgol

This comment has been minimized.

Show comment Hide comment
@Mithgol

Mithgol Aug 17, 2013

In my code I used break statements vertically aligned to case positions in order to see if a case is closed by the corresponding break:

switch(this.scheme){
   case 'netmail':
      netmailRequiredPart.call(this);
   break;
   case 'areafix':
   case 'echomail':
      areafixOrEchomailRequiredPart.call(this);
   break;
   case 'area':
   case 'fecho':
      areaOrFechoRequiredPart.call(this);
   break;
   case 'faqserv':
      faqservRequiredPart.call(this);
   break;
   case 'freq':
      freqRequiredPart.call(this);
   break;

   default: throw new Error(this.errors.UNKNOWN_SCHEME);
}

JSHint complained until the code became the following — with no (further) indent of case inside switch and with the indent of break the same as of the previous line (i.e. not negative indent):

switch(this.scheme){
case 'netmail':
   netmailRequiredPart.call(this);
   break;
case 'areafix':
case 'echomail':
   areafixOrEchomailRequiredPart.call(this);
   break;
case 'area':
case 'fecho':
   areaOrFechoRequiredPart.call(this);
   break;
case 'faqserv':
   faqservRequiredPart.call(this);
   break;
case 'freq':
   freqRequiredPart.call(this);
   break;

default: throw new Error(this.errors.UNKNOWN_SCHEME);
}

I had to add /* jshint indent: false */ because I had no way to check the indent size without the JSHint's indent style being enforced as well.

The new caseindent option would solve the problem of additional level of indent inside switch, but not the negative indent for break.

Can I have an additional option for negative indents on break statements?

Mithgol commented Aug 17, 2013

In my code I used break statements vertically aligned to case positions in order to see if a case is closed by the corresponding break:

switch(this.scheme){
   case 'netmail':
      netmailRequiredPart.call(this);
   break;
   case 'areafix':
   case 'echomail':
      areafixOrEchomailRequiredPart.call(this);
   break;
   case 'area':
   case 'fecho':
      areaOrFechoRequiredPart.call(this);
   break;
   case 'faqserv':
      faqservRequiredPart.call(this);
   break;
   case 'freq':
      freqRequiredPart.call(this);
   break;

   default: throw new Error(this.errors.UNKNOWN_SCHEME);
}

JSHint complained until the code became the following — with no (further) indent of case inside switch and with the indent of break the same as of the previous line (i.e. not negative indent):

switch(this.scheme){
case 'netmail':
   netmailRequiredPart.call(this);
   break;
case 'areafix':
case 'echomail':
   areafixOrEchomailRequiredPart.call(this);
   break;
case 'area':
case 'fecho':
   areaOrFechoRequiredPart.call(this);
   break;
case 'faqserv':
   faqservRequiredPart.call(this);
   break;
case 'freq':
   freqRequiredPart.call(this);
   break;

default: throw new Error(this.errors.UNKNOWN_SCHEME);
}

I had to add /* jshint indent: false */ because I had no way to check the indent size without the JSHint's indent style being enforced as well.

The new caseindent option would solve the problem of additional level of indent inside switch, but not the negative indent for break.

Can I have an additional option for negative indents on break statements?

@WolfgangKluge

This comment has been minimized.

Show comment Hide comment
@WolfgangKluge

WolfgangKluge Aug 17, 2013

Contributor

I know it's pure religion, but can you please explain why you unindent the break statement? It's a regular statement like return and if.

Maybe an example for better understanding. How would you reformat

switch (x) {
    case 2:
        if (something) {
            something = false;
            break;
        }
        otherthing = true;
        break;
    case 3:
       other thing = false;
        break;
}

For the option; this change was very easy. I don't think it's that easy to implement your wish (since I didn't understand it completely at this point)
Feel free to create a pull request... ;)

Contributor

WolfgangKluge commented Aug 17, 2013

I know it's pure religion, but can you please explain why you unindent the break statement? It's a regular statement like return and if.

Maybe an example for better understanding. How would you reformat

switch (x) {
    case 2:
        if (something) {
            something = false;
            break;
        }
        otherthing = true;
        break;
    case 3:
       other thing = false;
        break;
}

For the option; this change was very easy. I don't think it's that easy to implement your wish (since I didn't understand it completely at this point)
Feel free to create a pull request... ;)

@Mithgol

This comment has been minimized.

Show comment Hide comment
@Mithgol

Mithgol Aug 17, 2013

The idea is to see if a case is “closed” by its break.

switch (x) {
   case 2:
      if (something) {
         something = false;
         break;
      }
      otherthing = true;
   break;
   case 3:
      other thing = false;
   break;
}

The break inside the if(something){…} is not the “closing” break, but, as I see, that would need defining.

The “closing” break is on the same level of nesting and immediately precedes the next case or the end of their switch.

Mithgol commented Aug 17, 2013

The idea is to see if a case is “closed” by its break.

switch (x) {
   case 2:
      if (something) {
         something = false;
         break;
      }
      otherthing = true;
   break;
   case 3:
      other thing = false;
   break;
}

The break inside the if(something){…} is not the “closing” break, but, as I see, that would need defining.

The “closing” break is on the same level of nesting and immediately precedes the next case or the end of their switch.

@Mithgol

This comment has been minimized.

Show comment Hide comment
@Mithgol

Mithgol Aug 18, 2013

I feel now that I should have created a separate feature request, and even now it's not too late to do that.

Mithgol commented Aug 18, 2013

I feel now that I should have created a separate feature request, and even now it's not too late to do that.

@Mithgol

This comment has been minimized.

Show comment Hide comment
@Mithgol

Mithgol Aug 18, 2013

Created #1238.

Mithgol commented Aug 18, 2013

Created #1238.

@unwiredben

This comment has been minimized.

Show comment Hide comment
@unwiredben

unwiredben Aug 28, 2013

This would solve the #1 complaint about jshint-enforcement on the Enyo JS team.

This would solve the #1 complaint about jshint-enforcement on the Enyo JS team.

@sbmaxx

This comment has been minimized.

Show comment Hide comment
@sbmaxx

sbmaxx Sep 2, 2013

👍 i really want this feature to be merged ;)

sbmaxx commented Sep 2, 2013

👍 i really want this feature to be merged ;)

@bytasv

This comment has been minimized.

Show comment Hide comment
@bytasv

bytasv Sep 4, 2013

👍 please merge this one, it's a great feature that many are missing

bytasv commented Sep 4, 2013

👍 please merge this one, it's a great feature that many are missing

@mdoelker

This comment has been minimized.

Show comment Hide comment
@mdoelker

mdoelker Sep 4, 2013

👍 Would be great to see this feature make its way in. It's the one thing jshint keeps bugging me about that I won't "fix" ;)

mdoelker commented Sep 4, 2013

👍 Would be great to see this feature make its way in. It's the one thing jshint keeps bugging me about that I won't "fix" ;)

@kethinov

This comment has been minimized.

Show comment Hide comment
@kethinov

kethinov Sep 9, 2013

+1 for caseindent. This pull request should be merged. Come on maintainers. This is needed.

kethinov commented Sep 9, 2013

+1 for caseindent. This pull request should be merged. Come on maintainers. This is needed.

@willfarrell

This comment has been minimized.

Show comment Hide comment
@willfarrell

willfarrell Sep 10, 2013

+1 Please merge.

+1 Please merge.

@mgol

This comment has been minimized.

Show comment Hide comment
@mgol

mgol Sep 19, 2013

Contributor

Seems great, does what it should, has tests... @antonkovalyov, please, merge this one. :)

Contributor

mgol commented Sep 19, 2013

Seems great, does what it should, has tests... @antonkovalyov, please, merge this one. :)

@stephenmathieson

This comment has been minimized.

Show comment Hide comment
@stephenmathieson

stephenmathieson Sep 20, 2013

Contributor

+1

Contributor

stephenmathieson commented Sep 20, 2013

+1

@ignovak

This comment has been minimized.

Show comment Hide comment
@ignovak

ignovak Sep 24, 2013

+1 for merge

ignovak commented Sep 24, 2013

+1 for merge

@feugenix

This comment has been minimized.

Show comment Hide comment
@feugenix

feugenix Sep 24, 2013

+1, please merge

+1, please merge

@nerevar

This comment has been minimized.

Show comment Hide comment
@nerevar

nerevar Sep 24, 2013

+1

nerevar commented Sep 24, 2013

+1

@rndD

This comment has been minimized.

Show comment Hide comment
@rndD

rndD Sep 24, 2013

+1, merge please

rndD commented Sep 24, 2013

+1, merge please

@timonstr

This comment has been minimized.

Show comment Hide comment
@timonstr

timonstr Sep 24, 2013

+1

+1

@anton-rudeshko

This comment has been minimized.

Show comment Hide comment
@anton-rudeshko

anton-rudeshko Sep 24, 2013

+1

@eroshinev

This comment has been minimized.

Show comment Hide comment
@eroshinev

eroshinev Sep 24, 2013

+1

+1

@Flackus

This comment has been minimized.

Show comment Hide comment
@Flackus

Flackus Sep 24, 2013

+1

Flackus commented Sep 24, 2013

+1

@collapsus

This comment has been minimized.

Show comment Hide comment
@collapsus

collapsus Sep 24, 2013

+1

+1

@maetchkin

This comment has been minimized.

Show comment Hide comment
@maetchkin

maetchkin Sep 24, 2013

+1

+1

@yurich

This comment has been minimized.

Show comment Hide comment
@yurich

yurich Sep 24, 2013

+1

yurich commented Sep 24, 2013

+1

@arikon

This comment has been minimized.

Show comment Hide comment
@arikon

arikon Sep 24, 2013

Contributor

+1 for merge

Contributor

arikon commented Sep 24, 2013

+1 for merge

@corpix

This comment has been minimized.

Show comment Hide comment
@corpix

corpix Sep 24, 2013

+1 лойс

corpix commented Sep 24, 2013

+1 лойс

@mishanga

This comment has been minimized.

Show comment Hide comment
@mishanga

mishanga Sep 25, 2013

👍

👍

@tschieggm

This comment has been minimized.

Show comment Hide comment
@tschieggm

tschieggm Sep 25, 2013

+1

+1

@erunion

This comment has been minimized.

Show comment Hide comment
@erunion

erunion Sep 27, 2013

👍

erunion commented Sep 27, 2013

👍

@victor-homyakov

This comment has been minimized.

Show comment Hide comment
@victor-homyakov

victor-homyakov Sep 30, 2013

Contributor

👍

Contributor

victor-homyakov commented Sep 30, 2013

👍

@attomos

This comment has been minimized.

Show comment Hide comment
@attomos

attomos Oct 4, 2013

+1

attomos commented Oct 4, 2013

+1

@tadatuta

This comment has been minimized.

Show comment Hide comment
@tadatuta

tadatuta Oct 5, 2013

👍

tadatuta commented Oct 5, 2013

👍

@qubyte

This comment has been minimized.

Show comment Hide comment
@qubyte

qubyte Oct 5, 2013

Contributor

👍 If you merge this, I'll be able to remove a few hundred lines worth of /* jshint indent: false */!

Contributor

qubyte commented Oct 5, 2013

👍 If you merge this, I'll be able to remove a few hundred lines worth of /* jshint indent: false */!

@fxa

This comment has been minimized.

Show comment Hide comment
@fxa

fxa Oct 6, 2013

+1
switch case is the only reason, why I disabled the ident option at all

fxa commented Oct 6, 2013

+1
switch case is the only reason, why I disabled the ident option at all

@valueof valueof closed this in c64fb5c Oct 6, 2013

@valueof

This comment has been minimized.

Show comment Hide comment
@valueof

valueof Oct 6, 2013

Owner

I fixed this issue in a slightly different way. JSHint now looks at the switch statement, detects the style from its first token and uses that to check for indentation. This means that both styles will work without any additional options.

Owner

valueof commented Oct 6, 2013

I fixed this issue in a slightly different way. JSHint now looks at the switch statement, detects the style from its first token and uses that to check for indentation. This means that both styles will work without any additional options.

@kethinov

This comment has been minimized.

Show comment Hide comment
@kethinov

kethinov Oct 7, 2013

Thanks for working on this!

Unfortunately this case is still producing errors in jshint 2.1.11:

switch (condition) {
    case ABC:
        statements;
        break;
    case DEF:
        statements;
        break;
    default:
        statements;
        break;
}

kethinov commented Oct 7, 2013

Thanks for working on this!

Unfortunately this case is still producing errors in jshint 2.1.11:

switch (condition) {
    case ABC:
        statements;
        break;
    case DEF:
        statements;
        break;
    default:
        statements;
        break;
}
@Delapouite

This comment has been minimized.

Show comment Hide comment
@Delapouite

Delapouite Oct 7, 2013

It's because the fix hasn't been pushed to npm yet. 2.1.11 was released 17 days ago:

4aa8410

It's because the fix hasn't been pushed to npm yet. 2.1.11 was released 17 days ago:

4aa8410

@Mithgol

This comment has been minimized.

Show comment Hide comment
@Mithgol

Mithgol Oct 7, 2013

@antonkovalyov Please write a comment here to notify people after you publish a newer version of your npm package.

Mithgol commented Oct 7, 2013

@antonkovalyov Please write a comment here to notify people after you publish a newer version of your npm package.

@sandinmyjoints

This comment has been minimized.

Show comment Hide comment
@sandinmyjoints

sandinmyjoints Oct 15, 2013

👍

👍

jugglinmike added a commit to jugglinmike/jshint that referenced this pull request Oct 21, 2014

Fixes #935: Detect the switch indentation style and use that.
Look for the next token and try to determine whether the 'case'
statements should be indented or not from that.

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