search

squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.67k stars 1.48k forks source link

Allow ScopeIndentSniff to check for and fix tab-based indent rules #296

Closed aik099 closed 10 years ago

aik099 commented 10 years ago

The Generic_Sniffs_WhiteSpace_ScopeIndentSniff sniff indents the code (during auto-fixing process) using spaces and there is now way to configure it to use TABs instead.

When we're talking about finding errors in indentation-aware sniffs, then setting indent sniff parameter from 4 to 1 allowed to use TAB chars.

However, when auto-fixing comes in it's a different story, because we explicitly need to specify to PHP_CodeSniffer somewhere (or on per-sniff basis) what indentation sequence is ("4 spaces" or "1 tab").

This creates problems for example in https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/247#issuecomment-59536203 task (see video).

Here is the line when 1 space as indentation unit is hardcoded: https://github.com/squizlabs/PHP_CodeSniffer/blob/phpcs-fixer/CodeSniffer/Standards/Generic/Sniffs/WhiteSpace/ScopeIndentSniff.php#L269

gsherwood commented 10 years ago

The hard-coded indentation char is because that sniff only works with spaces. The --tab-width setting is just there to trick it into thinking it is checking spaces, along with many other sniffs.

Changing indents to use tabs is harder than doing spaces because tabs are variable width, but should be possible if you assume spaces are never used for indent in any way. But this is an enhancement that would need to be made to this sniff specifically as it would need to have an option to specifically checks for tab indents instead of spaces, allowing it to also fix tab indents instead of space indents.

gsherwood commented 10 years ago

I've added a new feature related to this. You can now choose to include parts of a ruleset only when phpcs or phpcbf is running. In this case, you'd only include the ScopeIndent sniff when phpcs is running, thus excluding it during auto-fixing by phpcbf:

<rule phpcs-only="true" ref="Generic.WhiteSpace.ScopeIndent" />

This should help until auto-fixing can be achieved for tab-based indents with this sniff.

aik099 commented 10 years ago

I hope this new syntax won't cause any trouble for dual-version standards, like mine (e.g. unknown attribute phpcs-only for rule node or something like that).

Would this new attribute also exclude that sniff from diff report created by phpcs?

aik099 commented 10 years ago

Changing indents to use tabs is harder than doing spaces because tabs are variable width, but should be possible if you assume spaces are never used for indent in any way. But this is an enhancement that would need to be made to this sniff specifically as it would need to have an option to specifically checks for tab indents instead of spaces, allowing it to also fix tab indents instead of space indents.

Exactly. Also PEAR's FunctionDeclaration Sniff needs to be updated to support that as well.

I hope this new syntax won't cause any trouble for dual-version standards, like mine (e.g. unknown attribute phpcs-only for rule node or something like that).

Checked myself, no errors during PHPCS 1.5.5 run.

gsherwood commented 10 years ago

I hope this new syntax won't cause any trouble for dual-version standards, like mine (e.g. unknown attribute phpcs-only for rule node or something like that).

I'm not that bad at PHP :) The attributes are completely optional.

aik099 commented 10 years ago

I'm not that bad at PHP :) The attributes are completely optional.

It's not what I meant :smile: . For example with DocCommentSniff I got the error when I mentioned it in the ruleset.xml and was using PHPCS 1.5.

gsherwood commented 10 years ago

Quick update: I think I have this working on my local machine. I've got a bit more testing to do tomorrow, but it is looking good for some proper real-world testing. You'll need to make sure your ruleset contains a tab-width (or you can set it on the command line) and that you set a property of the ScopeIndent sniff to tell it to use tabs.

All other sniffs will insert spaces for indentation, but the ScopeIndent sniff will fix indents with tabs if requested, and the DisallowSpaceIndent sniff will convert the remaining spaces into tabs. So the combination of those 2 sniffs should auto-fix the other corrections made by the standard.

aik099 commented 10 years ago

You'll need to make sure your ruleset contains a tab-width (or you can set it on the command line)

Won't the <arg> present in ruleset.xml trigger a Fatal Error for me, when used with PHPCS 1.5?

... and that you set a property of the ScopeIndent sniff to tell it to use tabs. ...

So you indent with spaces and then just replace N spaces with 1 tab? Then if after that a spaces in the line is left it's an error. Or in some cases, when exact = false in that sniff, then it maybe not an error.

Then something like this would be possible: phpcs --tab-width=2 --use-tabs .... and then 6 spaces will be converted into 3 tabs. Setting --tab-width to 1 would create undersided side effects, but that's up to user who will do that :)

aik099 commented 10 years ago

Looks like phpcs-only won't protect me, when I do phpcs --report=diff, because according to 5fffec3dc3d157e409a5b994f5e8e03d8111427d commit only the PHP_CODESNIFFER_CBF constant is checked and it's not set, when doing diff report.

gsherwood commented 10 years ago

Won't the present in ruleset.xml trigger a Fatal Error for me, when used with PHPCS 1.5?

It shouldn't, no. But you'd need to use the CLI arg --tab-width in earlier versions. If you forget, the sniffs will assume a tab width of 4 when they need to.

Looks like phpcs-only won't protect me, when I do phpcs --report=diff

That's correct. I am trying to decide if those rules should be enabled if you decide to only print a diff report. Hard to say because multiple reports can be printed to various files, and this check needs to be done before the ruleset is even parsed.

aik099 commented 10 years ago

It shouldn't, no. But you'd need to use the CLI arg --tab-width in earlier versions. If you forget, the sniffs will assume a tab width of 4 when they need to.

1 tab = 4 spaces sounds fair. However the default indent parameter for ScopeIndent sniff is 4 = 4 indent units, which somehow != 1 tab. I don't know it's a bug though. Because of that I always had to set --tab-width=0 and ident=1 (so it's 1 tab) to make it work.

gsherwood commented 10 years ago

I've now committed this feature. I've posted a comment with details at https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/247 , which I'll reproduce here:

I've just made a few commits for auto-fixing indents using tabs in the phpcs-fixer branch. The unit tests in there are > passing, but it could use some real-world testing if someone has time.

The best way to enforce tab indenting in PHP_CodeSniffer will now be to include the following in your ruleset:

<arg name="tab-width" value="4"/>
<rule ref="Generic.WhiteSpace.DisallowSpaceIndent"/>
<rule ref="Generic.WhiteSpace.ScopeIndent">
  <properties>
    <property name="indent" value="4"/>
    <property name="tabIndent" value="true"/>
  </properties>
</rule>

This enforces that tabs must be used for indenting via the DisallowSpaceIndent sniff (which I have just added auto-fixing > support to) and tells the ScopeIndent sniff to use tabs (4 spaces wide) when modifying indents (which I just added > support for). So this combination will replace space-based indents and also ensure they are lined up correctly. The > inclusion of the tab-width is important. If you leave it out, PHP_CodeSniffer will assume you want tabs to represent 4 > spaces.

You'll need the latest code on the phpcs-fixer branch to test this out because I have not done a release yet. If testing > goes well, I'll release this in 2.0.0RC4 next week.

Internally, what ends up happening with a tab indented file is that PHP_CodeSniffer replaces the tabs with spaces, using the tab-width setting to determine tab stops. DisallowSpaceIndent only looks at the original content before replacement, so it ignores these spaces. ScopeIndent uses the spaces to check indentation, but uses tabs when fixing the indents. Spaces that are less than the tab width remain as they are used for fine alignment.

aik099 commented 10 years ago

Unfortunately DisallowSpaceIndent sniff has side effect in PHPCS 1.x with such configuration: #299

gsherwood commented 10 years ago

You wouldn't use that configuration in 1.x because you aren't try to auto-fix anything, and because the ScopeIndent sniff doesn't support tab checks in there anyway.

PHP_CodeSniffer 1.x and 2.x are not compatible (hence the major version number change).

aik099 commented 10 years ago

How can create ruleset.xml that is compatible with both PHPCS versions when for example:

gsherwood commented 10 years ago

If that's what you want to do, you can't.

But tab-width has been there forever and was specifically designed to allow ScopeIndent to work on tab-based files. So it should have been used in 1.5 as well.

aik099 commented 10 years ago

ScopeIndent to work on tab-based files. So it should have been used in 1.5 as well.

PhpStorm's integration with PHP_CodeSniffer doesn't allow to specify --tab-width and I was forced to set indent parameter for all sniffs, that have it to 1. That way the default value of --tab-width, which is 0 worked pretty fine.

My backup plan was to do sudo phpcs --config-set tab_width 4 and return indent argument for all sniffs to 4. That should save the day, but I don't know how to fix DisallowSpaceIndent sniff in 1.5.5 and allow it to recognize TABs, that were replaced to spaces thanks to tab_width setting.