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

Slevomat Coding Standard 6.x #59

Merged
merged 1 commit into from
Dec 17, 2019
Merged

Slevomat Coding Standard 6.x #59

merged 1 commit into from
Dec 17, 2019

Conversation

kukulich
Copy link
Contributor

@kukulich kukulich commented Dec 5, 2019

I'm not sure if native type hints for properties should be disabled here.

@adaamz
Copy link

adaamz commented Dec 7, 2019

fixes #60

@VasekPurchart
Copy link
Member

Hi, thanks for the PR (looks great!). I just wanted to try it before merging and update the commit message to match the project style, but it looks like pushes are disabled for this PR? Could you please allow them (or change the commit msg to not be in the past/passive tense?). Thanks :)

@kukulich
Copy link
Contributor Author

@VasekPurchart Forcepushed.

Copy link
Member

@VasekPurchart VasekPurchart left a comment

Choose a reason for hiding this comment

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

I tested this on the 3.x branch and found out that there has been (probably undocumented - or at least I don't see it in release notes) a change/fix in SlevomatCodingStandard.Commenting.RequireOneLinePropertyDocComment.

Now it works probably for any comments? Or maybe just PHPDocs, but anyway this is no longer compatible with what is written in the standard (which uses this only for @var), so it needs to be removed.

Other option would be if there was an option on the sniff to apply this only for @var/properties, but I have checked and there probably is not.

Or am I missing something? Thanks :)

@kukulich
Copy link
Contributor Author

No change: https://github.com/slevomat/coding-standard/commits/master/SlevomatCodingStandard/Sniffs/Commenting/RequireOneLinePropertyDocCommentSniff.php

It always worked for any property phpdoc.

So it's not related to this update.

@VasekPurchart
Copy link
Member

VasekPurchart commented Dec 17, 2019

My guess was that some of the helpers might have changed the behavior , for example https://github.com/slevomat/coding-standard/commits/master/SlevomatCodingStandard/Helpers/DocCommentHelper.php ?

The only changes which were changed in the project I used for testing were:

  • Slevomat CS: 5.0.4 -> 6.0.3
  • PHPCS: 3.5.2 -> 3.5.3

So it is possible that this is caused by a change in PHPCS (although probably less likely), but since 3.5.3 is force by 6.x it does not matter anyway.

It always worked for any property phpdoc.

My bad I kind of overlooked that there is property in the title :) But there is definitely a bug, which threw me off before.

See 485efa0 + https://travis-ci.org/consistence/coding-standard/jobs/626363642#L305-L308 - the error is attributed to the line where a multiline (but with just one annotation) PHPDoc is starting, but which is rather a class PHPDoc. While the rest of the file is ok-ish - the missing PHPDoc property should be a different issue. The combination of these two seems to be the trigger.

So I think the sniff should stay, but there is a regression, which can be fixed in a patch release, so we do not need to solve it in this release.

Do you want me to create a separate issue or is the above reproducer enough?

@kukulich
Copy link
Contributor Author

@VasekPurchart VasekPurchart merged commit 1d6dc1f into consistence:master Dec 17, 2019
@VasekPurchart
Copy link
Member

Thanks again :)

@kukulich
Copy link
Contributor Author

You're welcome :)

@VasekPurchart VasekPurchart changed the title Slevomat CS updated to 6.0.0 Slevomat Coding Standard 6.x Dec 17, 2019
@VasekPurchart
Copy link
Member

Backported to 3.x and released as 3.10 .

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.

3 participants