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

RuleTestCase: Order errors #3931

Closed
wants to merge 1 commit into from
Closed

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Apr 11, 2025

as suggested in #3930 (comment)

just re-ordering error expectations. not other changes otherwise.

@ondrejmirtes
Copy link
Member

This is going to be painful for everyone 🙁 What about making it optional? We could make a new protected function shouldOrderErrors(): bool defaulting to false.

And then after the constructor change, the rules where the error order doesn't make sense, we could opt them into ordering.

And then change it for everyone in 3.0.

@staabm
Copy link
Contributor Author

staabm commented Apr 11, 2025

I think the problem is, that the impl of #3930 brings errors in a different order.

no matter whether we sort now, or sort only by flag: it will always affect already existing tests in case we can't "force" the order we had before #3930

but it feels like we did not have a "stable sort" order before so its not something we can enforce

@staabm
Copy link
Contributor Author

staabm commented Apr 11, 2025

we could make the "class-like-statements" sorting a feature-flagged only thing though..?
this would turn the whole scope from constructor narrowing a opt-in though

@staabm staabm closed this Apr 11, 2025
@staabm staabm deleted the order-errors branch April 11, 2025 14:43
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.

2 participants