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

Add positive restrictions next to negative ignores #76

Open
MetalArend opened this issue Feb 28, 2024 · 8 comments
Open

Add positive restrictions next to negative ignores #76

MetalArend opened this issue Feb 28, 2024 · 8 comments
Labels
enhancement New feature or request

Comments

@MetalArend
Copy link

Instead of saying "--ignore-shadow-deps --ignore-unused-deps --ignore-dev-in-prod-deps --ignore-prod-only-in-dev-deps", could we do "--scan-for-unknown-classes"?

@janedbal
Copy link
Member

This looks like you are trying to use this tool only to report unknown classes, but reporting unknown classes is more a side-feature of this tool. This check is present only to let people know that there are classes that cannot be assigned to any package and therefore we cannot check those for shadowness etc.

There are few options for you to simplify this usage:

And if you dont want the other features of this tool, there are also other tools that report autoloading issues, for example PHPStan (since level 0).

@MetalArend
Copy link
Author

Sorry, I was not clear about it, this was only an example. Any other category could be picked, and would be as interesting an example.

I was trying this package out, and trying to write some scripts to try to run this in CI. I want to fail on certain things, but only warn on others, so I tried to split the different reportings in different CI jobs.

The issue is that if I write a CI job based on ignores, and upgrade to a newer version that adds suddenly a new category, I can not be certain that it will only check a specific thing.

The example was for unknown classes, but any other singled out category is a good example.

The issue with writing scripts with ignores is that it is a possibly incomplete blacklist of what you don't want to check, instead of being able to whitest what you explicitly do want to check.

@MetalArend
Copy link
Author

I played around some more, to try to put your suggestion to good use into a makefile in our CI:

COMPOSER_DEPENDENCY_ANALYSER_POSSIBLE_CATEGORIES:=shadow-deps unused-deps dev-in-prod-deps prod-only-in-dev-deps unknown-classes

$(foreach category,$(COMPOSER_DEPENDENCY_ANALYSER_POSSIBLE_CATEGORIES),composer-dependency-analyser.$(category)):composer-dependency-analyser.%:
   @$(PHP) vendor/bin/composer-dependency-analyser $(addprefix --ignore-,$(filter-out $(*),$(COMPOSER_DEPENDENCY_ANALYSER_POSSIBLE_CATEGORIES)))

It turns out that when I add a specific --ignore-* flag, that the tool fails whenever that ignored flag was not even gonna throw: Error 'dev-dependency-in-prod' was globally ignored, but it was never applied., and thus the job still fails. :)

Hope you don't mind that I'm testing this tool in a big project. It's a great idea, thanks for the package, I love it. But splitting these into different CI jobs seems not feasible (yet).

@janedbal
Copy link
Member

I dont really understand why would you ignore error that is not present at all, but if you really need that, you can disable the "unused ignore" feature in the configuration file:

<?php

$config = new \ShipMonk\ComposerDependencyAnalyser\Config\Configuration();

return $config->disableReportingUnmatchedIgnores();

That way, any error like Error 'dev-dependency-in-prod' was globally ignored, but it was never applied. disappears.

@MetalArend
Copy link
Author

I dont really understand why would you ignore error that is not present at all Because at the time I run the checks on CI, I'm not sure if the error I want to be ignored will even be there at all, I only know that I want it to be ignored if it occurs.

Let's take a very specific example: I want to run checks on shadow dependencies and unused dependencies. I want my CI to fail hard on shadow dependencies, but on unused dependencies I want to only give a warning. So I want to run two separate checks, in two separate CI jobs.
To run the tool and only check (for example) the shadow dependencies, I have to pass the ignore flags for all the other possible checks. At the time I'm running a CI pipeline where I want to check only the shadow dependencies, I don't know yet if I need to ignore the check for "dev in prod dependencies".

After reading your last response, I got where I misunderstood the situation. Your initial proposal to use the ignore flags is not a correct solution, because those ignore flags are about ignoring errors, while what I want to do is not run those checks at all. Failing on something that is being ignored, that then never happens, is a very good take indeed, and similar to what other PHP QA tools do. But doing what I'm trying to do with them, is completely abusing them, and combining them with the disableReportingUnmatchedIgnores is basically even more abusing it. It will do what I want to do, without really being configured in the correct way. I'll do it that way for now, but I'm hoping things might be done differently in future versions. I could also add five different configuration files, but it would be awesome to keep it to a single configuration file, and be able to extend that configuration file per specific job with flags.

It could be interesting to add flags to basically do (almost) anything with flags that can also be configured by the configuration, and if implemented structurally, could still be okay from a maintenance burden standpoint.

Another possible solution could be to have commands, like phpstan's "analyse". So by default scan everything, but have a command "scan-for-shadow-deps" as well.

I'll keep an eye out on the package, because I really like the output and the ease of setup. Love the php config file as well ;)

@MetalArend
Copy link
Author

Btw, it's totally meant as feedback, not complaints, because this package rocks! We had an issue with a shadow dependency in our code suddenly vanishing because of upgrades, and this tool does not only solve that, but it also gave us already three dev only packages that were used as production dependencies, and some packages that were really not used.

(And some Symfony components that had to be configured, because they are being used by another dependency, but only if installed, so you still need to install them.)

So thanks a lot, all the love for creating this! Hooray twitter, for showing me your tweet :) And I'll add it to phpqa.io soon :D

@janedbal
Copy link
Member

Thanks, I'm glad you found this tool helpful!

I understand your usecase. Even though it is currently possible to accomplish what you need, this tool is not really designed for such usecase, so it gets clumsy. I think your approach is very uncommon so I'll wait if more people need this before implementing it. I hope you understand.

@MetalArend
Copy link
Author

I do understand. Looking forward where the journey brings you :)

@janedbal janedbal added the enhancement New feature or request label Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants