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

feat: loosen method sorting rules #104

Merged
merged 1 commit into from
Feb 6, 2023
Merged

feat: loosen method sorting rules #104

merged 1 commit into from
Feb 6, 2023

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Feb 6, 2023

Motivation

We use data providers in phpunit that are manually located bellow the test or bellow the set of tests that use it.

testA
providerA
testB
providerB
testCommonFoo
testCommonBar
providerCommon

Since phpunit v10 requires data providers to be static, the ClassStructure rule would detach them from the actual test resulting in:

providerA
providerB
providerCommon
testA
testB
testCommonFoo
testCommonBar

Also there has been a lot of discussion in past on how to order these methods.

Proposal

I propose to loosen the rule and sort methods only by visibility. Specifically:

  1. public
  2. protected
  3. private

The remaining attributes like abstract, final and static will not be taken into account and left for the user to do whatever he considers the best.

Copy link

@p4veI p4veI left a comment

Choose a reason for hiding this comment

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

V zasade asi nemam nic proti, ale trochu si myslim ze to pak nebude mit moc rad a vsude to bude jinak, jestli by nebyl lepsi ignore toho rule pro testy treba.. idk.

@simPod
Copy link
Contributor Author

simPod commented Feb 6, 2023

@p4veI

Ignore jen pro testy nejde. Musely by být separate configy.

Mi úplně jinej order nevadí, ale příp. bych to řešil přes rector or sth.

@simPod simPod merged commit 39a49e5 into 7.0.x Feb 6, 2023
@simPod simPod deleted the methods branch February 6, 2023 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants