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

Remove PHPCS and replace with ECS #191

Open
wants to merge 51 commits into
base: main
Choose a base branch
from
Open

Remove PHPCS and replace with ECS #191

wants to merge 51 commits into from

Conversation

schlessera
Copy link
Collaborator

@schlessera schlessera commented May 11, 2021

This PR removes PHPCS and replaces it with ECS - Easy Coding Standard.

This has the following advantages:

  • Use of both PHPCS and PHPCSF via one PHP config file
  • Advanced caching for quick rechecking during development
  • Possibility to check code snippets in markdown files

@schlessera schlessera added Testing Issues related with Unit, E2E, Smoke, and other testing requirements/needs Infrastructure Changes impacting testing infrastructure or build tooling labels May 11, 2021
@schlessera
Copy link
Collaborator Author

schlessera commented May 11, 2021

Overall, ECS seems to offer advantages over PHPCS for this repo. It is only supported right now from PHP 7.1 onwards, though.

So, if we want to go forward with this, we'll have to do one of:

  1. manually remove the ECS dependency on PHP < 7.1
  2. download the ECS Phar on-demand for code style checks
  3. bump PHP minimum version from PHP 5.6 to PHP 7.1

@schlessera
Copy link
Collaborator Author

Something to note: the original idea of trying to move to ECS is that it will allow the code generation for the validator spec to be run through the ECS fixed, which reliably enforces the exact same code style than for the rest of the code. This is unfortunately not easily done with PHPCS, as its fixing only covers a part of its ruleset.

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

LGTM, though I've never used ECS before

@CLAassistant
Copy link

CLAassistant commented Nov 25, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ schlessera
✅ westonruter
❌ github-actions[bot]
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Changes impacting testing infrastructure or build tooling Testing Issues related with Unit, E2E, Smoke, and other testing requirements/needs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants