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

Modernize codebase and add more code quality tools #37

Merged
merged 6 commits into from
Jul 28, 2024
Merged

Modernize codebase and add more code quality tools #37

merged 6 commits into from
Jul 28, 2024

Conversation

indy2kro
Copy link
Contributor

  • Added more code quality tools: phpcs, phpstan, rector
  • Updated project to be compatible with phpstan level 6
  • Added github actions for php 8.2 and 8.3
  • Added dependabot github action

@TavoNiievez
Copy link
Member

Hi @indy2kro

I agree with the changes you propose in your PR, however there were some that I took the liberty of undoing to continue (See e5de7b0):
For example, adding the rector dependency and its corresponding configuration file.
This specific dependency should not be part of the repository as it is not a linter itself but rather a tool to make refactor, which makes more sense when used locally.
I took the opportunity to remove PHP 8.0 support.
Regarding the typing you added, I had to undo the typing that specifies methods that are defined in an interface.
For congruence, the signature of the method must remain exactly the same in the implementation with respect to the interface.
That's why I lowered the phpstan level to 5 and undid those cases.
I also undid some naming changes that are not so relevant, and some specifications in conditionals that rector suggests but do not make sense in the logic being tested.

If you think I made a mistake in any of these changes please feel free to resubmit them in a new PR, arguing why.

Thank you very much for your contribution.

@TavoNiievez TavoNiievez merged commit 8a7f950 into Codeception:master Jul 28, 2024
3 checks passed
@indy2kro
Copy link
Contributor Author

Hello,

Regarding the rector observations, while the tool itself indeed can be used locally, I have learned over time that integrating inside the CI/CD can actually bring benefits - in particular through the PHP version upgrades rector can enforce best practices. However, I understand the reluctance and it is a personal preference in the end. I am happy to contribute even with the other changes which hopefully will bring benefits for others as well.

Thanks for the time and effort for review and merge!

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