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

Added test for phpcs #519

Closed
wants to merge 15 commits into from
Closed

Added test for phpcs #519

wants to merge 15 commits into from

Conversation

AltamashShaikh
Copy link
Contributor

Description:

Added test for phpcs

Review

@AltamashShaikh AltamashShaikh marked this pull request as draft April 9, 2024 05:22
@AltamashShaikh AltamashShaikh marked this pull request as ready for review April 9, 2024 05:22
@sgiehl
Copy link
Member

sgiehl commented Apr 9, 2024

@AltamashShaikh I guess it should be better to have a separate action for this. The test yml file can be auto generated with a command. Doing that would throw away the PHPCS part.
Also using strict PSR12 won't easily work without introducing braking changes. This is the CS we are currently using for core: https://github.com/matomo-org/matomo/blob/5.x-dev/phpcs.xml
Note: We are currently working on enabling a few more rules to get closer to PSR12... At some point we need to create a new repository that holds our PHPCS rules, and maybe even allows to add custom rules in the future. Every repo would than need to require those rules and simply run PHPCS with them.

@AltamashShaikh
Copy link
Contributor Author

@AltamashShaikh I guess it should be better to have a separate action for this. The test yml file can be auto generated with a command. Doing that would throw away the PHPCS part. Also using strict PSR12 won't easily work without introducing braking changes. This is the CS we are currently using for core: https://github.com/matomo-org/matomo/blob/5.x-dev/phpcs.xml Note: We are currently working on enabling a few more rules to get closer to PSR12... At some point we need to create a new repository that holds our PHPCS rules, and maybe even allows to add custom rules in the future. Every repo would than need to require those rules and simply run PHPCS with them.

@sgiehl Yes will move to a separate action and as suggested will make the related changes

@michalkleiner
Copy link
Contributor

I would exclude vendor and all the prefixed dependencies from the code check, and only apply the action to actual plugin code.
As mentioned before in a meeting, and as Stefan also suggests, longterm we should aim at having something like matomo-org/matomo-coding-standards repo that can be shared across core and plugins and cover other areas like JSLint or prettier rules etc., not just PHP. It's a start here though.
Github actions can also be reused from other repo, so here we could simply reference the core repo action (check examples in https://docs.github.com/en/actions/using-workflows/reusing-workflows, mainly https://docs.github.com/en/actions/using-workflows/reusing-workflows#calling-a-reusable-workflow).

@AltamashShaikh
Copy link
Contributor Author

e reused from other repo, so here we could simply reference the core repo action (

@michalkleiner I can try your suggestions and update you accordingly 👍

@AltamashShaikh AltamashShaikh marked this pull request as draft August 20, 2024 05:19
@AltamashShaikh
Copy link
Contributor Author

Closing this in favor of #547

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