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

OperatorSpacing sniff: Defer to upstream sniff for improved results. #989

Merged
merged 1 commit into from Jun 20, 2017

Conversation

Projects
None yet
3 participants
@jrfnl
Contributor

jrfnl commented Jun 20, 2017

Up to now, this sniff was based on a copy of a very old and outdated version of the upstream sniff.
The upstream and downstream sniffs had diverged quite widely code-wise, but not intention-wise.

I've run both the WordPress.WhiteSpace.OperatorSpacing sniff as well as its upstream version Squiz.WhiteSpace.OperatorSpacing over both the WPCS unit tests for this sniff as well as the Squiz unit tests for this sniff.

While there were differences in the results, they were for the better in favour of the upstream version of the sniff.

The unit test file has been adjusted to only test for the WPCS additional token.

Fixes #988

Summary of the differences:

1. "Boolean not" was not handled by the upstream sniff:

This was added to the WPCS version in #67

if (!$var ) {}

Fixed: by overloading the register() method and adding the token to the sniff.

2. Operator spacing after - (minus) was not verified in the WPCS version

This was previously disabled to prevent the sniff from triggering on code like:

$a = -1;
return -1

However, as a side effect, this also prevented the sniff from triggering & fixing this:

$result = 1  -        2;

The + operator can be used in a similar vain, but was unaccounted for in WPCS, so the below code would trigger an error with the WPCS version of the sniff:

$a = +1;

Fixed: the current upstream sniff already accounts for these situations and will not throw errors for it (example 1 and 3), but will correctly throw an error for example 2.

3. Checking for spacing around the & (bitwise and) was removed from the WPCS version

I suspect this was done to prevent false positives for & being used as a reference.
This also meant however that spacing around real "bitwise and" was not being checked, so code like this would not trigger any errors nor be fixed:

if ($result&4 && $result  &  4) {}

Fixed: the upstream sniff correctly accounts for and bows out if & is being used as a reference operator, so there should be no problem with that check.

4. New lines should always be acceptable instead of a space

Code like the below should not trigger the sniff:

$y = 1
   + 2  
   - 3;

Fixed: since PHPCS 2.2.0 / early 2015 the upstream sniff has had a $ignoreNewLines property which can be set to true to handle this. Fixed by setting that property to true in our child class.

OperatorSpacing sniff: Defer to upstream sniff for improved results.
Up to now, this sniff was based on a copy of a very old and outdated version of the upstream sniff.
The upstream and downstream sniffs had diverged quite widely code-wise, but not intention-wise.

I've run both the `WordPress.WhiteSpace.OperatorSpacing` sniff as well as its upstream version `Squiz.WhiteSpace.OperatorSpacing` over both the WPCS unit tests for this sniff as well as the Squiz unit tests for this sniff.

While there were differences in the results, they were for the better in favour of the upstream version of the sniff.

@JDGrimes JDGrimes merged commit 648c0dd into develop Jun 20, 2017

2 checks passed

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

@JDGrimes JDGrimes deleted the feature/operator-whitespace-defer-to-upstream branch Jun 20, 2017

*
* Last synced with base class December 2008 at commit f01746fd1c89e98174b16c76efd325825eb58bf1.

This comment has been minimized.

@GaryJones

GaryJones Jun 20, 2017

Member

2008?! Impressive!

@GaryJones

GaryJones Jun 20, 2017

Member

2008?! Impressive!

This comment has been minimized.

@jrfnl

jrfnl Jun 20, 2017

Contributor

LOL, yup, that was when the original was cloned and never synced again ;-)

Until now that is ;-)

@jrfnl

jrfnl Jun 20, 2017

Contributor

LOL, yup, that was when the original was cloned and never synced again ;-)

Until now that is ;-)

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