Page MenuHomePhabricator

Set tab-width in the base ruleset file
Closed, ResolvedPublic

Description

Since we use tabs to indent lines, we should tell PHPCS how many spaces a tab equals to. Usually (e.g. in IDEs), a tab corresponds to 4 spaces, and we should tell this to PHPCS by setting tab-width.

Not setting this option causes some whitespace-related issues to behave badly, see T243529. People upstream suggested setting the tab width to let the InterfaceWrongIndent sniff and others behave correctly.

However, there's an important side-effect in doing so: the LineLength sniff. Right now, the following line:

\t\t\tFOO

(where \t is a tab) is counted as having 6 characters (3 tabs + 3 letters). By setting tab-width to 4, it will be counted as having 15 characters (4*3 + 3). Hence, we'll start getting lots of new warnings about long lines, see test run. Of note, I've always found it weird to have tabs counted as 1, but I don't know whether it's intentional/there are historical reasons to do so. For sure, it makes more sense (IMHO) to count a tab as 4 characters when computing line length.

Either way, I see three possible outcomes:

  1. We don't set tabWidth, but then we'll have to disable InterfaceWrongIndent, and possibly other space-related sniffs.
  2. We set tabWidth, but we also increase the max line length (e.g. by 8-10 chars, assuming that the average indent is around 2.5-3 tabs).
  3. We set tabWidth, and also fix all the long lines.

IMHO, 3 is the best option, but 2 would be fine for now.

Event Timeline

Yes, it does. Feel free to disable the rule globally (in the phpcs.xml file), right now it's useless.

I agree, having tabs count as one character has always felt odd to me: it means that lines can appear longer than 100 chars, but not result in a warning.

If we were to increase the line length to 120 (which is what PSR12 says) then there are only 6 lines in MediaWiki includes/ that are currently longer than that.

Setting the tab-width will also mean we can enable Generic.WhiteSpace.ScopeIndent, which would be good (there is a bunch of incorrectly-indented code).

	<rule ref="Generic.WhiteSpace.ScopeIndent">
		<properties>
			<property name="tabIndent" value="true" />
			<property name="exact" value="true" />
		</properties>
	</rule>

Change 579119 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/tools/codesniffer@master] Add tab-width=4 and increase line length to 120

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

Change 579448 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/core@master] Reduce the length of 6 long lines of code

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

Change 579119 merged by jenkins-bot:
[mediawiki/tools/codesniffer@master] Add tab-width=4 and increase line length to 120

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

Change 579448 merged by jenkins-bot:
[mediawiki/core@master] Reduce the length of 6 long lines of code

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

Daimona assigned this task to Samwilson.

Change 655681 had a related patch set uploaded (by Reedy; owner: Samwilson):
[mediawiki/tools/codesniffer@19.x] Add tab-width=4 and increase line length to 120

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

Change 655681 merged by Reedy:
[mediawiki/tools/codesniffer@19.x] Add tab-width=4 and increase line length to 120

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