Fix linter settings

[azaozz]

Currently ESLint throws error on:

if ( mainCondition === 1 ) {
   if ( secondaryCondition > 1 ) {
      // do stuff
   } else {
      // do other stuff
   }
} else {
   if ( secondaryCondition < 10 ) {
      // do stuff
   } else {
      // do other stuff
   }
}

This is coming from the no-lonely-if setting: https://eslint.org/docs/rules/no-lonely-if which says that you should never have an if inside an else. That may be true in some cases, but in the above example makes the nesting irregular which makes the whole thing harder to read/understand.

Think this rule should be disabled. It is not in the WP coding standards for a good reason :)

[azaozz]

Oh, the page in the ESLint's docs also says: "Disable this rule if the code is clearer without it" :)

[gziolo]

Is there any particular code example you couldn’t use because of this rule?

By the way. You can always disable this rule per a bunch of lines or per file with a code comment. We do it from time to time when it makes sense.

[tofumatt]

I would leave this setting on in general and disable it for the times it does make sense, as @gziolo said. In general this kind of nesting looks like a code smell and is likely a sign things could be clearer/refactored.

I think conditionally disabling it is better than disabling it all the time.

[azaozz]

Is there any particular code example you couldn’t use

Yep, trying to make this bit: 287f623#diff-59b3e108ef76b1269ef0cecf2b417214R348 more readable here: 078b3db#diff-59b3e108ef76b1269ef0cecf2b417214R352.

You can always disable this rule per a bunch of lines...

Right, was just about to do that but then thought the rule doesn't really make much sense. I mean, who writes conditionals like:

if ( a ) {
  // do stuff
} else {
  if ( b ) {
     // hmm, redundant indentation?
  }
}

That seems to be why the rule exists, but imho in cases with several if() else if() else if() else it shouldn't mangle the last bit as it breaks the indentation (and the readability) for no good reason.

I would leave this setting on in general and disable it for the times it does make sense

Sure, that works too, although don't think we need it in the first place :)

[tofumatt]

Huh, in your example it might be easier to have less nesting I think. What about:

if ( align === 'center' ) {
	// When the image is centered, show both handles.
	showRightHandle = true;
	showLeftHandle = true;
} else if ( isRTL ) {
	// In RTL mode the image is on the right by default.
	// Show the right handle and hide the left handle only when it is aligned left.
	// Otherwise always show the left handle.
	if ( align === 'left' ) {
		showRightHandle = true;
	} else {
		showLeftHandle = true;
	}
} else if ( align === 'right' ) {
	// Show the left handle and hide the right handle only when the image is aligned right.
	showLeftHandle = true;
} else {
	// Otherwise show the right handle.
	showRightHandle = true;
}

But actually I don't like that the conditional varies… I think the eslint rule is working here to lessen the indentation/nesting of the code. You are essentially doing a switch case on align here… I think your example would be better expressed as:

switch ( align ) {
	case 'center':
		// When the image is centered, show both handles.
		showLeftHandle = true;
		showRightHandle = true;
		break;
	case 'left':
		// In RTL mode the image is on the right by default.
		// Show the right handle (and hide the left handle) when it is aligned left.
		if ( isRTL ) {
			showRightHandle = true;
		} else {
			// Show the left handle in LTR, for left-aligned images.
			showLeftHandle = true;
		}

		break;
	case 'right':
	default:
		// Show the left handle (and hide the right handle) when the image is aligned right in RTL.
		// If no align value is set we show the handle on the right side as well.
		if ( isRTL ) {
			showLeftHandle = true;
		} else {
			// Show the right handle for right-aligned images in LTR.
			showRightHandle = true;
		}

		break;
}

There's a lot of nesting here and I think keeping which conditional we're checking consistent makes the code easier to follow.

If you're comfortable with just leaving it on in general for now I think it's worth it–I know it would stop me from writing badly-nested code and refactoring. If it doesn't apply in this case and you're happy to ignore the rule and explain why (a link to this issue would be nice in the comments) I'll close this for now 😄

[azaozz]

might be easier to have less nesting I think...
But actually I don't like that the conditional varies… I think the eslint rule is working here to lessen the indentation/nesting of the code.

Yep, that's exactly what I'm trying to avoid. There are two "sets" of conditions, one is isRTL, the other is align. The align is dependent on isRTL. When the user has not aligned the image, it is "left" in LTR, and "right" in RTL mode. To keep that distinction it is pretty good to have these two "conditionals" with different indentation.

The switch looks a bit better, but... a switch is always a bit harder to read than a "simple" conditionals :)
Also the example above doesn't work. align has 4 states: left, right, center, and undefined, where the undefined is equal to "left" in LTR and "right" in RTL. I know, it's confusing... That's why was trying to make it easier to read :)

Thinking to keep the exception here. The switch will need another block and will seem too repetitive perhaps.