Coding-standard: Removing dependency on codesniffer

[ostrolucky]

I believe there is no need at this time to explicitly specify requirement for codesniffer. Use of this repository is just to get some additional checkers.

I would like to require this repository in my project without need to install codesniffer and all its dependencies with it, because codesniffer I use is installed in separate location, in docker image, therefore codesniffer provided via this repo is useless.

I believe there isn't much risk someone would want to use this repo without knowing what is codesniffer

[ondrejmirtes]

I believe that the dependency on CodeSniffer here is justified. It's not guaranteed that the sniffs will work with whatever version of CodeSniffer you have installed in the separate location, they are tested only with the version specified in composer.json: "^3.0.1". The sniffs are meant to be installed alongside CodeSniffer.

[ostrolucky]

Right, that's why I have written it's not needed at this time. Is there a reason to believe sniffs will be incompatible in near future? That would be pretty big BC break on CodeSniffer side. I think it's safe to remove it now and put it back when such situation actually happens, in YAGNI style.

The sniffs are meant to be installed alongside CodeSniffer.

Correct, that's why I don't see strong need for this package to install it. It's assumed user already has CodeSniffer installed.

I just got and idea though. How about replacing current require with conflicts?

[kukulich]

1. Our sniffs depend on PHPCS directly because the sniffs implement interface \PHP_CodeSniffer\Sniffs\Sniff.
2. The pretty big BC break in PHPCS happened just in its version 3 so we have to require version 3. Actual version of our sniffs is not compatible with PHPCS 2.
3. In PHPCS 3.0.0 was a bug that broke our sniffs so we have to require exactly version ^3.0.1.
4. "It's assumed user already has CodeSniffer installed" - that's not true, newcomer can start directly with this repository.
5. require is standard composer way how to declare dependencies.

So I will close this issue :)

[soullivaneuh]

"It's assumed user already has CodeSniffer installed" - that's not true, newcomer can start directly with this repository.

On reverse side, you are currently forcing user to install a binary tool and the many dependencies it needs on project side, even if not wanted.

Is the same issue than requiring phpunit on a phpunit plugin.

It would be better to require it as dev deps for your tests and explain the possible solutions on the documentation.

[soullivaneuh]

Another possible sample: https://github.com/djoos/Symfony-coding-standard/blob/4bbb23250bbfef2020e2fd7aebf94a0007fdfb5a/composer.json#L28-L34

And it does not force the end user to install the tool inside his project.

[alcaeus]

@Soullivaneuh as soon as you use a class from a package, e.g. by instantiating a class, extending a class or implementing an interface, you create a dependency on said package and thus should add an explicit requirement for it. If it's an "optional" dependency (e.g. only used if explicitly required by the user) you may get away with using suggest and conflict (to exclude incompatible versions) with an optional require-dev dependency for your own tests. However, this is extremely risky since there's nothing telling the user they can't use class X unless precondition Y (e.g. a specific package is installed) is fulfilled.

As such, the only correct course of action is explicitly declaring your dependencies, as is done here. See also: composer-require-checker.

[soullivaneuh]

However, this is extremely risky since there's nothing telling the user they can't use class

You may always put one a two check with proper errors on your cli.

I understand composer dependency is better about that, but dev tools should not interfere with project dependencies IMO.

[alcaeus]

If you use extends Something or implements Something, you're no longer using a dev-tool: you're using a dependency of your project and should version it properly.

[github-actions[bot]]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.