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.

Sign up for GitHub

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

Jump to bottom

Add attribute sniffs #296

Closed
wants to merge 1 commit into from
Closed

Add attribute sniffs #296

wants to merge 1 commit into from

Conversation

michnovka
Copy link
Contributor

Add new attribute sniffs to doctrine standard

@michnovka michnovka requested a review from a team as a code owner October 18, 2022 17:29
@derrabus
Copy link
Member

Our test fixtures should be updated to reflect the proposed change.

@michnovka
Copy link
Contributor Author

@derrabus have a look if I did it properly

simPod
simPod reviewed Oct 18, 2022
View reviewed changes
Copy link
Contributor

@simPod simPod left a comment

Choose a reason for hiding this comment

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

You'll also need to adapt the report summary

A TOTAL OF 429 ERRORS AND 0 WARNINGS WERE FOUND IN 46 FILES

and all the patches

tests/expected_report.txt Outdated Show resolved Hide resolved
@kukulich
Copy link
Contributor

I think SlevomatCodingStandard.Attributes.RequireAttributeAfterDocComment should be added too so PHPStan (psalm maybe too) works as expected, see nikic/PHP-Parser#762

@michnovka
Copy link
Contributor Author

is there some tool which generates the patches? seems tedious to do this by hand...

@michnovka
Copy link
Contributor Author

I am trying the makefile update-compatibility-patch-XX but this gives me

error: patch failed: tests/expected_report.txt:42
error: tests/expected_report.txt: patch does not apply
make: *** [/home/superuser/scripts/doctrine-coding-standard/Makefile:12: update-compatibility-patch-72] Error 1

@michnovka
Copy link
Contributor Author

I am sorry, I tried to make the patches work, but I only created mess. I have forced pushed back the latest version which has expected_report.txt for PHP8.1 working fine, please somebody do the dirty work and finish the patches for 7.2-8.0.

@kukulich I have also added the SlevomatCodingStandard.Attributes.RequireAttributeAfterDocComment and relevant test

@greg0ire
Copy link
Member

Yeah, updating the patches is a pain, I'll give it a try. Also, I'll retarget to 11.0, which is the correct branch according to https://www.doctrine-project.org/projects/doctrine-coding-standard/en/stable/reference/index.html#testing

@greg0ire
Copy link
Member

Also, I'll retarget to 11.0,

Ugh I should have started with that 🤦

@greg0ire greg0ire changed the base branch from 10.0.x to 11.0.x October 18, 2022 19:56
simPod
simPod reviewed Oct 18, 2022
View reviewed changes
tests/php72-compatibility.patch
@@ -530,7 +530,7 @@ diff --git a/tests/input/arrow-functions-format.php b/tests/input/arrow-function
index d3903ff..8a358e8 100644
--- a/tests/input/arrow-functions-format.php
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why there are so many changes 🤔 something is wrong. Should I clean it up?

Copy link
Member

@greg0ire greg0ire Oct 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe what I did was wrong? How do you usually proceed? What I did was:

  1. edit the patch to remove the hunk about the expected report, so that it applies
  2. apply the patch
  3. set the correct php version in phpcs.xml.dist
  4. run vendor/bin/phpcs $(find tests/input/* | sort) --report=summary --report-file=tests/expected_report.txt

Copy link
Contributor

Choose a reason for hiding this comment

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

Depends, I sometimes do it by hand. I have re-applied patches on 11.x branch and there are little changes only.

I guess this must be run on related php version vendor/bin/phpcs $(find tests/input/* | sort) --report=summary --report-file=tests/expected_report.txt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simPod I experienced the same issue, this is why I asked for somebody to finish it. Yes, it seems PHP version running the phpcs is relevant. I guess it also installs different versions of composer dependencies, especially for e.g. 7.2

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe nobody here likes the patches but that's the best we've got. I'll look into it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, there are sniffs that are enabled/disabled by detected PHP version - and it’s intentional here.

Copy link
Member

Choose a reason for hiding this comment

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

I forced the php version in step 3 of my process (otherwise the tests would be red)

Copy link
Contributor

@simPod simPod Oct 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I have opened in as #297 #298 since I cannot push here. We can either merge that (I kept the author) or someone with permissions do some commit shuffling.

Copy link
Member

Choose a reason for hiding this comment

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

You mean #298

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. I have done +1 but somebody opened a PR meanwhile :D

@simPod simPod mentioned this pull request Oct 18, 2022
Merged
@greg0ire greg0ire closed this Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greg0ire greg0ire greg0ire left review comments

@simPod simPod simPod approved these changes

@kukulich kukulich kukulich left review comments

@derrabus derrabus derrabus approved these changes

@SenseException SenseException SenseException approved these changes

@ostrolucky ostrolucky ostrolucky approved these changes

Assignees
No one assigned
Labels
BC break New rule
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@michnovka @derrabus @kukulich @greg0ire @simPod @ostrolucky @SenseException