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

Faulty core fixes: the case of the moving space #977

Closed
jrfnl opened this Issue Jun 17, 2017 · 2 comments

Comments

Projects
None yet
1 participant
@jrfnl
Contributor

jrfnl commented Jun 17, 2017

Summary:

While squizlabs/PHP_CodeSniffer#1404 definitely is a huge improvement in the spaces -> tabs fixing, I'm still seeing an issue with this sniff when there is a combination of tabs and spaces for indentation and the spaces are not there for alignment.

Most likely cause: Generic.WhiteSpace.DisallowSpaceIndent

Example:

File: ./src/wp-admin/update.php line 127 (and in lots of other places too)

Source:

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

Fixed as:

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

Expected fix:

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

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Aug 10, 2017

Contributor

A fix for this has now been pulled upstream: squizlabs/PHP_CodeSniffer#1599
The PR also addresses a secondary issue, i.e. that subsequent lines of a multi-line non-doc-block comment were not fixed by the upstream sniff.

Analysis of this bug and the effect of the upstream fix

To analyse the effect, I've used a combination of running the new WordPress.WhiteSpace.PrecisionAlignment sniff - PR #1099 - and both the old and the improved version of the upstream Generic.WhiteSpace.DisallowSpaceIndent sniff against the WP Core /src directory using the phpcs.xml.5.dist ruleset.

Generic.WhiteSpace.DisallowSpaceIndent: Old (upstream master) Improved (upstream PR)
Original precision alignment issues found 399 399
Nr of issues fixed by the upstream sniff 423 in 90 files 440 in 92 files (+17)
Precision alignment issues found after fixing by upstream sniff 539 (+140) 310 (-89)
Contributor

jrfnl commented Aug 10, 2017

A fix for this has now been pulled upstream: squizlabs/PHP_CodeSniffer#1599
The PR also addresses a secondary issue, i.e. that subsequent lines of a multi-line non-doc-block comment were not fixed by the upstream sniff.

Analysis of this bug and the effect of the upstream fix

To analyse the effect, I've used a combination of running the new WordPress.WhiteSpace.PrecisionAlignment sniff - PR #1099 - and both the old and the improved version of the upstream Generic.WhiteSpace.DisallowSpaceIndent sniff against the WP Core /src directory using the phpcs.xml.5.dist ruleset.

Generic.WhiteSpace.DisallowSpaceIndent: Old (upstream master) Improved (upstream PR)
Original precision alignment issues found 399 399
Nr of issues fixed by the upstream sniff 423 in 90 files 440 in 92 files (+17)
Precision alignment issues found after fixing by upstream sniff 539 (+140) 310 (-89)
@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Nov 27, 2017

Contributor

Wowie! Just in time for the core patch, the upstream PR for this issue has just been merged ;-) /cc @pento

Contributor

jrfnl commented Nov 27, 2017

Wowie! Just in time for the core patch, the upstream PR for this issue has just been merged ;-) /cc @pento

@jrfnl jrfnl closed this Nov 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment