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

Sniff/isClassProperty: add unit tests covering multi-property definitions #583

Merged
merged 1 commit into from
Jun 10, 2018

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Feb 16, 2018

Verifies and safe-guards that multi-property definitions are handled correctly.


Note: the build is currently not passing for the PHPCS check on the PHPCompatibility files because of a known upstream bug.
See squizlabs/PHP_CodeSniffer#1898 and squizlabs/PHP_CodeSniffer#1899 for more details.
This is expected to be fixed soon.

The fix for the upstream issue has been merged, so the builds are passing again.

@jrfnl jrfnl added this to the 8.1.1 milestone Feb 16, 2018
@jrfnl jrfnl requested a review from wimg February 16, 2018 17:40
Copy link
Member

@wimg wimg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrfnl Could you have a look at the comment I provided during review ?

// Multi-property declarations.
class MyClass {
/* Case 27 */
public $varA = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this a typo ? Semi colon instead of comma ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching that. I've fixed up the original commit to change it to a comma.

@jrfnl jrfnl force-pushed the feature/isClassProperty-additional-unit-tests branch from ddf7c0d to 286703a Compare March 18, 2018 16:31
…ions

Verifies and safe-guards that multi-property definitions are handled correctly.
@jrfnl jrfnl force-pushed the feature/isClassProperty-additional-unit-tests branch from 286703a to 879b22d Compare March 18, 2018 17:29
@jrfnl
Copy link
Member Author

jrfnl commented Mar 18, 2018

Rebased to get the build to pass. (CS change in the mean time would interfere otherwise)

@jrfnl jrfnl added the PR: quick merge PR only contains relatively simple changes label Jun 8, 2018
@wimg wimg merged commit 4d55aca into master Jun 10, 2018
@jrfnl jrfnl deleted the feature/isClassProperty-additional-unit-tests branch June 10, 2018 21:48
@jrfnl jrfnl removed the PR: quick merge PR only contains relatively simple changes label Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants