Page MenuHomePhabricator

Require indentation of CASE statements in PHP code
Closed, ResolvedPublic

Description

Per this discussion, the goal of this task is to identify parts of code that do not have proper indentation for the CASE statement (inside of a SWITCH statement), and add the indentation. Then, make the indentation of CASE mandatory (currently, it is optional).

The proposal is to add a level indentation for each case statement. Thus instead of:

switch ( $var ) {
case 'a':
    // some thing
    break;
default:
    echo "not handled";
}

We would:

switch ( $var ) {
    case 'a':
        // some thing
        break;
    default:
        echo "not handled";
}

Event Timeline

Huji created this task.Dec 11 2017, 1:00 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 11 2017, 1:00 AM
Huji claimed this task.Dec 11 2017, 1:06 AM
Huji triaged this task as Lowest priority.
Huji added subscribers: Anomie, Esanders.
Huji renamed this task from Require indenting of CASE statements in PHP code to Require indentation of CASE statements in PHP code.Dec 11 2017, 1:22 AM

Change 397216 had a related patch set uploaded (by Huji; owner: Huji):
[mediawiki/core@master] Require indentation of CASE statements in PHP code

https://gerrit.wikimedia.org/r/397216

Huji added a comment.Dec 11 2017, 3:34 AM

Besides the patch I just submitted (for MW core), there are 94 more files that need to be dealt with. I will wait for this one to be merged before working on the files in MW extensions.

hashar updated the task description. (Show Details)Dec 11 2017, 9:15 AM
hashar added a subscriber: hashar.

That is better tracked via MediaWiki-Codesniffer.

Code style is enforced via PHP_CodeSniffer for which we have custom rules in a composer package: https://packagist.org/packages/mediawiki/mediawiki-codesniffer (build from mediawiki/tools/codesniffer.git in Gerrit).

There are some informations on: https://www.mediawiki.org/wiki/Continuous_integration/PHP_CodeSniffer

And the coding convention / style guide at: https://www.mediawiki.org/wiki/Manual:Coding_conventions/PHP

A change such as https://gerrit.wikimedia.org/r/397216 , should have the appropriate rule to enforce the layout.

I am not a fan of that extra level of indentation :D

Huji added a comment.Dec 12 2017, 1:09 AM

@hashar: Currently MediaWiki-Codesniffer does not have a rule for this indentation, does it? If so, would you expect me to first submit a patch to that project to introduce the rule? Note that even then, nothing will change with 397216 (as there currently is no exclusion for the rule that doesn't exist anyway). So technically, 397216 should not have "the appropriate rule" (we don't include rules, we exclude them, right?)

I think it's fine to do both in parallel. I think hashar was just pointing out that if there's no codesniffer rule to enforce it, it will easily regress.

I think it's fine to do both in parallel. I think hashar was just pointing out that if there's no codesniffer rule to enforce it, it will easily regress.

Similarly, it is probably easier to add and release the updated ruleset first. Then, in the commit that bumps the mediawiki-codesniffer version, we can run the automatic fixer for any existing violations.

However, before we put too much effort in either of these steps, we should probably first ensure consensus. In particular, https://www.mediawiki.org/wiki/Manual:Coding_conventions has always explicitly stated that both ways are commonly used. Speaking from my own experience, I believe switch cases are most often written without the extra indentation for cases. Would be good to get some data and/or other opinions on this. Note though that counting raw matches in all code is not always a good measure for consensus because not every occurrence was written separately by someone (many matches will originate from the same source, and then copied-pasted as base; and/or adjusted over time to people's own preference or sense of consistency within a certain context).

Personally, I'm fine with either, with a soft preference towards not indenting (based on current habits).

I don't know if we're doing a poll of any kind, but based on Ed's assessment I'd now favour towards indenting (consistency with JS, and because it's a curly brace block of which every other type we already indent). I previously thought the JS side was still inconsistent as well, and I was thinking about the colon-block instead of the curly-block.

Huji added a comment.Dec 12 2017, 12:52 PM

Okay, so plan of action is:

  1. Add this feature to CodeSniffer
  2. Modify the current submitted patch to also include a bump in the codesniffer version

Correct?

Sounds good to me.

Change 397216 merged by jenkins-bot:
[mediawiki/core@master] Require indentation of CASE statements in PHP code

https://gerrit.wikimedia.org/r/397216

Huji added a comment.Dec 19 2017, 1:58 PM

@Esanders you merged this patch. Is it because someone else added the feature to CodeSniffer and I am not aware of it? If not, I'll try to work on the patch this weekend.

Huji removed Huji as the assignee of this task.Jan 2 2018, 12:28 AM

@Esanders you merged this patch. Is it because someone else added the feature to CodeSniffer and I am not aware of it? If not, I'll try to work on the patch this weekend.

Not that I'm aware of. Looking forward to the CS patch :)

Change 613729 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/core@master] Fix switch/case indentation per [[mw:Coding conventions]]

https://gerrit.wikimedia.org/r/613729

Change 613734 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/tools/codesniffer@master] Add PSR2.ControlStructures.SwitchDeclaration

https://gerrit.wikimedia.org/r/613734

Unfortunately the above rule only has an error/fix for the indentation of the terminating statement (break/throw/return), despite what the documentation says (https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Standards/PSR2/ruleset.xml#L205), but that will still let the user know they need more indentation.

Change 613729 merged by jenkins-bot:
[mediawiki/core@master] Fix switch/case indentation per mediawiki coding conventions

https://gerrit.wikimedia.org/r/613729

Personally, I disagree with making this a strict rule. There is nothing wrong with either of the two switch shown in the task description above. From my point of view, we do have a common code style for one and only one reason: To make code easier to read. And while I agree that the extra indention in the 2nd example does make the code a tiny little bit easier to read, it's not like the 1st example is unreadable.

I don't think I have ever seen somebody arguing about this in a code review. What's the problem with some switch being formatted like this?

hashar removed a subscriber: hashar.Jul 20 2020, 1:04 PM
Huji added a comment.Jul 20 2020, 1:25 PM

I could say the same for how we indent the insides of a for loop or an if clause. The "there is nothing wrong" argument is non-falsifiable.

I don't have any strong feelings about this myself. I am just pointing out that we should not analyze this from the perspective of right and wrong. None of the other indentations are done because they are "right" or "necessary". They are perceived as useful, and I'm not sure if that is backed by hard evidence or not. What is proposed in this task is also based on perceptions of increased readability, not hard evidence.

I could say the same for how we indent the insides of a for loop or an if clause.

No, you can't. Removing the indention from a for or if leaves nothing, while the actual code in each case up there in the first example is still indented.

Change 613734 merged by jenkins-bot:
[mediawiki/tools/codesniffer@master] Add PSR2.ControlStructures.SwitchDeclaration

https://gerrit.wikimedia.org/r/613734

DannyS712 closed this task as Resolved.Sun, Sep 13, 11:43 PM
DannyS712 assigned this task to Esanders.
DannyS712 removed a project: Patch-For-Review.