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

Config codings styles in composer.json from project #23

Closed
fadoe opened this issue Apr 8, 2017 · 1 comment
Closed

Config codings styles in composer.json from project #23

fadoe opened this issue Apr 8, 2017 · 1 comment

Comments

@fadoe
Copy link

fadoe commented Apr 8, 2017

Problem/Motivation

Your plugin only works, when the type in the composer.json from the codings standard is "phpcodesniffer-standard". The problem: this is not a requirement and not a standard.

Expected behaviour

I can install any coding standard that use the Codesniffer-Standard. There must not be a type "phpcodesniffer-standard" in composer.json from the coding standard. E.g. "move-elevator/symfony-coding-standard"

Actual behaviour

Only packages with the package type "phpcodesniffer-standard" working with this plugin. E.g. "frenck/php-compatibility".

Steps to reproduce

Require "frenck/php-compatibility" in composer.json as coding standard and it works.
Require "move-elevator/symfony-coding-standard" in composer.json as coding standard and it doesn't work.

Proposed changes

Add a key "phpcodesniffer-standards" in the composer.json from the project and add all packages that are a coding standard:

    "require-dev": {
        "dealerdirect/phpcodesniffer-composer-installer": "^0.3.2",
        "frenck/php-compatibility": "^7.1",
        "move-elevator/symfony-coding-standard": "^2.0"
    },
    "extra": {
        "phpcodesniffer-standards": [
            "move-elevator/symfony-coding-standard"
        ]
    }

In Plugin::getPHPCodingStandardPackages:

        $extra = $this->composer->getPackage()->getExtra()["phpcodesniffer-standards"];

       // check if $extra is set

        $codingStandardPackages = array_filter(
            $this->composer->getRepositoryManager()->getLocalRepository()->getPackages(),
            function (PackageInterface $package) use ($extra) {
                if ($package instanceof AliasPackage) {
                    return false;
                }

                if (in_array($package->getName(), $extra)) {
                    return true;
                }

                return $package->getType() === Plugin::PACKAGE_TYPE;
            }
        );

Output from phpcs -i:

PHP CodeSniffer Config installed_paths set to /home/falk/Projekte/phpcodesniffer-composer-project/vendor/frenck/php-compatibility,/home/falk/Projekte/phpcodesniffer-composer-project/vendor/move-elevator/symfony-coding-standard/Standards
@Potherca
Copy link
Member

Although we appreciate the effort that has gone into reporting this, we do not intend to support this feature.

The main reason are:

  1. In it's first incarnation, this package used to have this functionality. That didn't work too well in various scenarios (including meta-packages) and with some edge-cases. If you would like to know more details on this topic, I can ask @frenck to expand on it.
  2. This package is explicitly designed to work "as-is" as an alternative to other implementations that either do weird things installing sniffs or require actions from users. We intend to honor this design. There is only one requirement for it to work "out of the box" and that is the phpcodesniffer-standard declaration in the composer.json
  3. Adding this feature would introduce more complexity here. More logic would need to be added in other places (for instance in the upcoming feature to support relative paths). This would make this package harder to maintain for (from our perspective) very little gain. Requiring other packages to declare themselves as phpcodesniffer-standard adds zero code, no complexity and no extra maintenance overhead.

As an alternative to adding this feature we would suggest opening a pull-request in the mentioned repository, explaining why declaring phpcodesniffer-standard in the composer.json and using this package would be beneficial to them (and their users).

A package can declare phpcodesniffer-standard without requiring this installer and nothing changes at all. The package will be installed just as it always has, as a composer "library" package. We do not see any reason for maintainers not to honor such a PR.

We've set up a standard template that can be used for opening PR's explaining the workings and/or the benefit.[1]

Again, we really do appreciate the time and effort that has gone in to reporting this be we are declining to change the way the package works.

[1]: Feedback is welcome.

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

No branches or pull requests

2 participants