magento / marketplace-eqp

Magento 1.x Coding Standard
http://docs.magento.com/marketplace/user_guide/Resources/pdf/Extension_Quality_Program_Overview.pdf
MIT License
225 stars 68 forks source link

Please specify the version of PHP_CodeSniffer in composer.json #5

Closed danslo closed 7 years ago

danslo commented 8 years ago

The version that comes with Magento 2.1 (1.5.3) doesn't work with 1.0.1 of this repo.

lenaorobei commented 8 years ago

We added requirements to our README.md file.

danslo commented 8 years ago

But can you add them to composer.json? That's kinda what it's for ;-)

ikruchynskyi commented 8 years ago

@danslo Do you mean that we need to add it into "require" section? In case someone already has PHPCodesniffer - why he need to install another one? The main idea of custom standards is that they should be an addition to already exists Codesniffer standards.

danslo commented 8 years ago

Yeah, that's exactly what composer is for. If your repository doesn't work without that specific dependency, it has a _require_ment on it.

By your logic, why specify "php": ">=5.6.0", someone might already have PHP 5.4.

ed007m commented 8 years ago

@danslo Some of our sniffs doesn't work on php5.4.

danslo commented 8 years ago

And your code sniffs don't work at all without PHP_CodeSniffer.

ed007m commented 8 years ago

And what if someone already use PHP_CodeSniffer?

danslo commented 8 years ago

In any of the above cases, marketplace-eqp repo should still be specific about its own requirements.

ed007m commented 8 years ago

There is one more case - when PHP_CodeSniffer lives as it's own application. In that case person needs only to specify path to MEQP standards or path to other standard they what. Without installing PHP_CodeSniffer for each standard. As example you can look how PhpStorm works. After specifing path to phpcs, all what you need is just to set path to your custom standard and it is ready to use.

Marketplace-eqp is not a standalone application (for now). In future, everything can change, and, ofcourse, we will add this requirement to composer. But now, it's just a standard like many others.

ed007m commented 8 years ago

@danslo I'm sorry for late response. Actually, now we are reviewing your recommendation with our architects. And we will notify you as soon as.

ed007m commented 8 years ago

@danslo we reviewed your proposition and PHP_CodeSniffer version will be added to composer.json file in next release.

Thank you for reporting.

danslo commented 8 years ago

Thank you!