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

Keep phpstan strict rules testing #9424

Merged
merged 6 commits into from
Nov 20, 2024

Conversation

mvorisek
Copy link
Contributor

Keep phpstan strict rules for CI.

Too strict rules with many matches should be ignored by regex instead, but overall the quiality should be asserted, the rules are great!

@mvorisek mvorisek marked this pull request as ready for review April 22, 2024 13:25
@mvorisek
Copy link
Contributor Author

@alecpl can this PR be merged?

@alecpl
Copy link
Member

alecpl commented May 30, 2024

I appreciate your commitment, but I'm against this change.

@mvorisek
Copy link
Contributor Author

mvorisek commented Jun 3, 2024

Given all the detected issues are valid, I kindly ask if this PR can be merged in order to maintain the project in unarguably a better shape.

In my opinion, the few needed @phpstan-ignore comments (of real issues to be fixed someday) are worth and the few ignores of too strict errors in phpstan config does not hurt either.

@pabzm
Copy link
Member

pabzm commented Jun 3, 2024

@alecpl Can you explain why you're against this?
I don't know if I agree with every rule, but in general I would prefer to have advanced automated syntax/style checking, as it helps contributors to conform to the wanted rules and thus requires a little less attention from you (or the team).

@mvorisek
Copy link
Contributor Author

@alecpl Can you explain why you're against this?

In #9424 (comment) I have summarized the reasons why the changes in this PR are good and help to maintain this project in better shape. It is fine to ignore some too strict rules, but not all of them, some are legit.

@mvorisek mvorisek force-pushed the keep_phpstan_strict_testing branch 2 times, most recently from 00f4bcd to 019a8ce Compare June 17, 2024 11:24
@mvorisek
Copy link
Contributor Author

@alecpl I have rebased this PR, let's merge it.

@alecpl
Copy link
Member

alecpl commented Nov 19, 2024

Ok, let's do it. After the conflict is resolved.

@mvorisek mvorisek force-pushed the keep_phpstan_strict_testing branch from 019a8ce to 6a89015 Compare November 19, 2024 21:51
@alecpl alecpl merged commit efcdce8 into roundcube:master Nov 20, 2024
16 checks passed
@mvorisek mvorisek deleted the keep_phpstan_strict_testing branch November 20, 2024 11:05
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.

3 participants