Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic/DisallowSpaceIndent: multi line comments and the case of the moving space #1599

Conversation

Projects
None yet
2 participants
@jrfnl
Copy link
Contributor

commented Aug 10, 2017

Allow the sniff to fix space indents for multi-line /* */ comments

Given this code sample (all lines indented with spaces):

    /*
     * Some text
     */

The tokenizer tokenizes this code as:

T_WHITESPACE T_COMMENT
T_COMMENT
T_COMMENT

In other words, the whitespace at the start of subsequent comment lines is tokenized as part of the T_COMMENT.

In effect, this meant that these lines would not throw the appropriate error, nor be fixed by this sniff.

This commit fixes that.

Includes unit tests.


Fix the case of the moving space

This is one of the issues I came across during reviews related to the WP core code style fix effort - WordPress/WordPress-Coding-Standards#977

Given this code:

	 		wp_die( $api ); // Tab - Space - Tab - Tab

with tab width set to 4, wp_die() would start in column 13.

The sniff was currently fixing this to:

			 wp_die( $api ); // Tab - Tab - Tab - Space

which changed the start column to 14 and changed the "space hidden before a tab" to precision alignment.

This commit fixes that and is a further iteration building onto the improvements in #1404 and reverts the change in the fixes for line 22 and 24.

The above code will now be fixed as:

			wp_die( $api ); // Tab - Tab - Tab

Notes:
* As the tabs in whitespace at the start of T_INLINE_HTML and T_COMMENT tokens is not replaced by spaces in the content by the tokenizer, this has to be done within the sniff to determine what the correct length of the whitespace should be.
* Basing the correction on the space-based length of the whitespace allows for fixing with higher precision.
* Incidentally, this also fixes one of the metrics being recorded incorrectly. For in-depth details of the effect on the metrics of this fix, please see: https://gist.github.com/jrfnl/5e2d75894c8e60a8f314b9fcb0ad3f62
* The tabWidth is now set in the unit test file.

It goes without saying, that for the fixes and the metrics to be accurate, the sniff has to be run with a tab width set.

@jrfnl

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2017

Rebased for merge conflicts.

@jrfnl jrfnl force-pushed the jrfnl:feature/disallowspaceindent-multi-line-comments branch from 29c9155 to 4efa1a7 Oct 17, 2017

jrfnl added some commits Aug 9, 2017

DisallowSpaceIndent: Allow the sniff to fix space indents for multi-l…
…ine `/* */` comments

Code sample:
```php
    /*
     * Some text
     */
```

The tokenizer tokenizes this code as:
```
T_WHITESPACE T_COMMENT
T_COMMENT
T_COMMENT
```

In other words, the whitespace at the start of subsequent comment lines is tokenized as part of the `T_COMMENT`.

In effect, this meant that these lines would not throw the appropriate error, nor be fixed by this sniff.

This commit fixes that.

Includes unit tests.
DisallowSpaceIndent: Fix the case of the moving space
Given this code:
```php
	 		wp_die( $api ); // Tab - Space - Tab - Tab
```
with tab width set to `4`, `wp_die()` would start in column 13.

The sniff was currently fixing this to:
```php
			 wp_die( $api ); // Tab - Tab - Tab - Space
```
which changed the start column to 14 and changed the "space hidden before a tab" to precision alignment.

This commit fixes that and is a further iteration building onto the improvements in 1404.

The above code will now be fixed as:
```php
			wp_die( $api ); // Tab - Tab - Tab
```

Notes:
* As the tabs in whitespace at the start of `T_INLINE_HTML` and `T_COMMENT` tokens is not replaced by spaces in the `content` by the tokenizer, this has to be done within the sniff to determine what the correct length of the whitespace should be.
* Basing the correction of the space-based length of the whitespace allows for fixing with higher precision.
* Incidentally, this also fixes one of the metrics being recorded incorrectly. For in-depth details of the effect on the metrics of this fix, please see: https://gist.github.com/jrfnl/5e2d75894c8e60a8f314b9fcb0ad3f62
* The `tabWidth` is now set in the unit test file.

@jrfnl jrfnl force-pushed the jrfnl:feature/disallowspaceindent-multi-line-comments branch from 4efa1a7 to eb3c55f Nov 22, 2017

@jrfnl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2017

Rebased for merge conflicts & code style...

@gsherwood gsherwood merged commit eb3c55f into squizlabs:master Nov 26, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

gsherwood added a commit that referenced this pull request Nov 26, 2017

@gsherwood

This comment has been minimized.

Copy link
Member

commented Nov 26, 2017

I never even noticed that the unit test for this was wrong with that mixed space/tab indent. Thanks a lot for these changes.

@jrfnl jrfnl deleted the jrfnl:feature/disallowspaceindent-multi-line-comments branch Nov 27, 2017

@jrfnl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2017

You're welcome. Thanks for merging this.

@jrfnl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2017

@gsherwood Greg, if you have a chance, it'd be great if you could also have a look at #1607 which basically fixes the same thing for the comments for the DisallowTabIndent sniff. Also, PR #1762 which I pulled last night, is a tiny change to the same two sniffs for another bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.