Generalise this package into any standard

GaryJones commented 7 years ago

This package looks at running PHPCS with a certain standard, but this is too broad and the wrong focus IMO. It should define that PHPCS to run in a git hook - not also how PHPCS should run.

PHPCS can run with a phpcs.xml.dist config file, which should generally expect to live in a project's root directory.

This config file then specifies how exactly PHPCS should run - which directories and files to run over / ignore, which arguments, and more importantly, which standards, sniffs and custom properties to use.

My typical project would have a PHPCS config that referenced PSR2 or WordPress (but with WordPress-VIP excluded), along with ObjectCalisthenics and PHPCompatibility, as well as custom properties for one or more of them (like whether it's a WP theme, or which prefixes and textdomain should be checked for, or which versions of PHP it should be compatible with etc.)

As such, instead of having a php-composter-phpcs-psr2 package, and a php-composer-phpcs-wordpress package and others doesn't make sense.

It would be far better to have a more general php-composter-phpcs package which simply run phpcs, optionally with an extras key that could override the default config (otherwise, let PHPCS find and use the phpcs.xml.dist or phpcs.xml).

schlessera commented 7 years ago

I generally agree, but the problem is that we need to pull in the dependencies needed by a specific rule set as well. So, if you want to check for WPCS, you need to pull in WPCS as a dependency as well. It's not enough to just have it be mentioned in the phpcs.xml[.dist] file.

Do you have specific thoughts about getting around this problem?

GaryJones commented 7 years ago

I don't think that's a problem for this package. My (perhaps naive) vision would be for me to have the following in my theme's composer.json:

    "require": {
        "php": "^7.1",
        "brightnucleus/config": "^0.4",
        "composer/installers": "^1.3",
        "gamajo/genesis-theme-toolkit": ">=0.5",
        "roave/security-advisories": "dev-master"
    },
    "require-dev": {
        "brain/monkey": "^2",
        "phpunit/phpunit": "^6.2",
        "dealerdirect/phpcodesniffer-composer-installer": "^0.4",
        "php-composter/php-composer-phpcs": "dev-master",
        "php-composter/php-composer-phpunit": "dev-master",
        "object-calisthenics/phpcs-calisthenics-rules": "^3",
        "wimg/php-compatibility": "dev-master",
        "wp-coding-standards/wpcs": "dev-develop"
    },

Those dev dependencies (in order) help me to:

patch WP functions,
run unit tests,
automatically detect and register code standards,
automatically add PHPCS to pre-commit hook,
automatically add PHPUnit to pre-commit hook,
use object calisthenics standard,
use PHP Compatibility standard,
use WPCS standard (at least this one pulls in PHPCS itself)

As you can see - I've already decided which dependencies I need, and how to use them in phpcs.xml.dist - I just want this package to do the automatic-hooking, just like the dealerdirect composer plugin package does the automatic registering of code standards.

Does that make sense?

schlessera commented 7 years ago

Yes, it does make sense, but it is moving towards a different goal than I initially had with PHP-Composter.

If you set your composer.json up like above, there's not mch missing from actually doing everything by hand.

One of the goals for PHP-Composter was that you tell your composer.json what you need, not how you achieve it.

Think of something like: "I want this to be checked against WPCS".

What you have above is more like: "I want this to be checked against something with PHPCS. I want this to have a PHPCS standards installation plugin. I want this to have the WPCS standard installed."

It sure is more generalized, but I think that both approaches might make sense in different contexts.

I'll think some more about this. Maybe it just makes sense to have 1 generalized and multiple specific PHPCS packages...?

GaryJones commented 7 years ago

I want this to be checked against WPCS

Unlike PSR2, which is generally all or nothing, it's not recommended to check against "WordPress" (since it includes "WordPress-VIP", and has custom properties that should be set) without configuring it further via the phpcs.xml.dist.

Maybe it just makes sense to have 1 generalized and multiple specific PHPCS packages...

That would be preferable (IMO), because then it's is flexible for those who say "I want this to be checked against WPCS in this way, and these specific PHP versions".

schlessera commented 7 years ago

The PSR-2 one was actually just an easy one to be able to run practical tests with this system. Once I've got everything cleaned up now and the API is somewhat stable, I'll start adding lots more.

GaryJones commented 7 years ago

The PSR-2 one was actually just an easy one to be able to run practical tests with this system.

Understood - I just wanted to flag the issue before you started work on php-composter/php-composer-wordpress without considering that actual usage of standards outside of PSR-2 is not as straight forward (all or nothing) as using PSR-2.

johnbillion commented 6 years ago

Chatted very briefly with Alain about this. It sounds like there is space for both approaches - a generic PHPCS package which operates according to "My project is set up like this, and I'd like a pre-commit hook to run PHPCS according to my configuration", and also a WCPS-specific one which operates as a single dependency along the lines of "I want my code to be checked by WPCS". I think this makes sense.

php-composter / php-composter-phpcs-psr2

Generalise this package into any standard #1