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

Adds test for phpcs using Matomo coding standards, #PG-3751 #547

Open
wants to merge 31 commits into
base: 5.x-dev
Choose a base branch
from

Conversation

AltamashShaikh
Copy link
Contributor

Description:

Adds test for phpcs using Matomo coding standards
Fixes: #PG-3751

Review

@AltamashShaikh AltamashShaikh marked this pull request as draft August 19, 2024 03:30
Copy link
Contributor

@snake14 snake14 left a comment

Choose a reason for hiding this comment

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

Looking really good 👍 The only concern I have is the code sniffer dependency (vendor/) being committed. That doesn't seem like something that should be included in production.

@AltamashShaikh
Copy link
Contributor Author

Looking really good 👍 The only concern I have is the code sniffer dependency (vendor/) being committed. That doesn't seem like something that should be included in production.

@snake14 Slevomat requires phpcodensiffer

@snake14
Copy link
Contributor

snake14 commented Aug 22, 2024

@snake14 Slevomat requires phpcodensiffer

@AltamashShaikh Shouldn't Slevomat be require-dev too?

@AltamashShaikh
Copy link
Contributor Author

@snake14 As discussed I gitignored all those libraries

@snake14
Copy link
Contributor

snake14 commented Aug 22, 2024

@snake14 As discussed I gitignored all those libraries

Thank you. That looks a lot better. You could probably ignore the two new files in vendor/bin/ as well, but it's not a big deal.

@AltamashShaikh
Copy link
Contributor Author

@snake14 As discussed I gitignored all those libraries

Thank you. That looks a lot better. You could probably ignore the two new files in vendor/bin/ as well, but it's not a big deal.

@snake14 Done :)

@AltamashShaikh AltamashShaikh mentioned this pull request Aug 27, 2024
11 tasks
phpcs.xml Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@michalkleiner
Copy link
Contributor

I think it would be great to split this PR into two - one where we add the config and any necessary dependencies, and leave the reformatting for a separate one. Together it makes it harder to review.

composer.json Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
Copy link
Contributor

@snake14 snake14 left a comment

Choose a reason for hiding this comment

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

Aside from Michal's latest comments, I think things are looking good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants