-
Notifications
You must be signed in to change notification settings - Fork 72
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 an option to check the development code. #163
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch per-se is OK: code is tested, and can maybe be improved, but not massively.
I'm more worried about the use-case though: if we really (which is an important question) want to support require-dev
analysis, we need to be crystal clear about the use-case, since we were very restrictive so far (on purpose) to avoid downstream having broken dependency graphs.
In practice we need to write down when this flag is to be used.
src/ComposerRequireChecker/FileLocator/LocateComposerPackageDirectDependenciesSourceFiles.php
Outdated
Show resolved
Hide resolved
src/ComposerRequireChecker/FileLocator/LocateComposerPackageDirectDependenciesSourceFiles.php
Outdated
Show resolved
Hide resolved
src/ComposerRequireChecker/FileLocator/LocateComposerPackageSourceFiles.php
Outdated
Show resolved
Hide resolved
The use case for this additional option is not immediately obvious to me. I'd very much read about a real world story where this additional check finds something - and what it would be. I'm very happy this tool exists, and I want to emphasize that it's dedicated purpose makes it directly useful without using any command line options at all (once you know how it's output should trigger your next actions). Originally, we had 1 code base and 1 list of dependencies, and they had to "match". Now we'd introduce 2 code bases and 2 lists of dependencies, and they add up to 4 relations, some of them probably must still "match", but others do not need to. In fact, after painting it on my whiteboard, I'd say this: And if we could get it coded like this, then the option would actually make sense to add, because the prod code check would remain the same and still be triggering errors in the same cases, thus removing the need to profusely explain the differences in the readme. |
The use case is the following : finding indirect dependencies in test code, for very same reason you want to find them in the production one. Someone once told me to treat the test code the same way you treat the production one. However, in this version, |
ef14320
to
0812320
Compare
Ok, so I've tried to slap a couple of graphs together to explain the use cases. The current CRC check ensures that the production code of the checked projects only depends on production code of direct, production dependencies. E.g.: The checked project only requires "foo", so only production code from the foo package is allowed. bar is a dev dependency, and quz, tester, mouse and builder are indirect dependencies, so they should not be referenced from the code. In the case of a dev check, the proposed change ensures that the dev code (e.g. test and builder code, mostly) only depends on the production code of direct production and dev packages. E.g.: The checked project requires both "foo" and "bar" for its development, so production code from both there packages is allowed. quz, tester, mouse and builder are indirect dependencies, so they should not be referenced from the code. As you can see, there are invariants:
|
Just some \DIRECTORY_SEPARATOR issues.
0812320
to
03338e3
Compare
I would appreciate if this was supported, I arrived here because at first I thought I have a bug somewhere in the setup. My realword usecase: I know about this tool for a long time and when I recently encountered missing dependency in tests, which broke the build for a contributor and then I spent quite some time tracking where in the dependencies it broke. I thought: "This would never happened if I was using CRC". Today, when I was adding CRC I wanted to test this case, to see it fail and nothing happened, which was not really obvious to me, since it is not really mentioned in the Readme. I think the usecase is exactly the same as with the "production" code, just as this issue describes: "you should put your requires for tests to require-dev to avoid breaking in the future". Since should be about half of the code (maybe more), I think it would be beneficial to check them as well. Plus in case of libraries a lot of times, there are more dependencies for tests than src code.
Yes, this is exactly my situation :)
@Ocramius if implemented using the rules described in the comments above I do not understand how this could have this effect? |
In our case it led to this problem, because we did not require |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick review, did not dig deeper in the logic as I'm not familiar with the project.
@@ -51,6 +51,18 @@ If this is already done, run it like this: | |||
composer-require-checker check /path/to/your/project/composer.json | |||
``` | |||
|
|||
### Check development code and dependencies (optional) | |||
|
|||
By default, Composer require checker only checks the source code listed in the `autoload` section of `composer.json` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, Composer require checker only checks the source code listed in the `autoload` section of `composer.json` | |
By default, Composer Require Checker only checks the source code listed in the `autoload` section of `composer.json` |
To check the *development* code, use the `--dev` option. This will scan the source code from the `autoload-dev` section and will search for symbols in dependencies from both the `require` and `require-dev` | ||
section. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If max-width is somehow applied (looking at lines above), then it should be rather:
To check the *development* code, use the `--dev` option. This will scan the source code from the `autoload-dev` section and will search for symbols in dependencies from both the `require` and `require-dev` | |
section. | |
To check the *development* code, use the `--dev` option. This will scan the source code from the `autoload-dev` section | |
and will search for symbols in dependencies from both the `require` and `require-dev` section. |
matrix: | ||
- PHP_VERSION: '7.2' | ||
COMPOSER_FLAGS: '--prefer-stable --prefer-lowest' | ||
- PHP_VERSION: '7.3' | ||
- PHP_VERSION: '7.4' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHP version matrix needs to be updated.
Or maybe AppVeyor is not required at this point? Github Actions support Windows 🤔.
'dev', | ||
null, | ||
InputOption::VALUE_NONE, | ||
'check that the development sources (i.e. tests) have not indirect dependencies' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'check that the development sources (i.e. tests) have not indirect dependencies' | |
'check that the development sources (i.e. tests) does not have indirect dependencies' |
See the explanation in the README.md change.