Use Puli discovery #34
Conversation
sagikazarmark
force-pushed
the
puli_discovery
branch
4 times, most recently
from
2725bd0 to
7f11469
Compare
|
Big Puli 👍! |
| build/ | ||
| vendor/ | ||
| composer.lock | ||
| phpspec.yml | ||
| phpunit.xml | ||
| puli.json |
There was a problem hiding this comment.
Did you not commit the puli.json file by intention? Just like composer.json, this file should usually be committed and distributed with the package.
There was a problem hiding this comment.
I generally agree, but this package is supposed to be installed in other packages or an application. There are no bindings or binding types in this package, so only the "lock" functionality would be generated into the json file, which IMO doesn't make sense here, but would make sense in the application.
Why is the package list generated in the json file in the first place? I though the json file is some kind of configuration and I expect it doesn't contain anything else. The generated package list could be part of some kind of lock file, couldn't it?
There was a problem hiding this comment.
Ok, makes sense then.
The package list is in there since, at least in principle, Puli is independent of Composer. You could install additional packages manually or generally use Puli without Composer, if that makes sense for your project. So puli.json duplicates some parts of composer.json, however those parts are synchronized by the Composer plugin.
There was a problem hiding this comment.
Actually I was thinking about a puli.lock file. In cases like this, a puli.json might not even be necessary in a project/application, only a puli.lock where non-configuration stuff can go. Even if you add stuff manually there, it is rather an "environmental" (aka. project dependent) thing, which depends on external resources, while you might change a configuration based on internal project changes.
There was a problem hiding this comment.
Puli works differently than Composer. There's no need for a lock file in Puli.
You are right that in this case, a puli.json is not needed (but neither is a lock file).
There was a problem hiding this comment.
Well, calling it lock is probably not straightforward and multiple configuration can be confusing. Also, packages for composer are only generated when the composer plugin is installed and it should only be done in an application, am right? With other words: package configuration, like composer packages should only be in an application, but not in reusable packages. Am I correct?
There was a problem hiding this comment.
Not necessarily. If your packages contain integration tests that require f.i. Puli's Discovery service and certain binding types/bindings to be loaded from other packages, then those packages must be loaded into Puli as well, otherwise the binding types/bindings are not found.
There was a problem hiding this comment.
I see. Are those packages loaded into the application where the package is installed afterwards? Because I probably don't want integration testing binding in the application where I use the package.
There was a problem hiding this comment.
Cool. Thanks for clarification.
|
This is a great improvement! Good to use something for discovery that’s already good at discovery, like Puli. 😉 @sagikazarmark Let’s squash and merge? |
Add Puli CLI as dev dependency Replace actual implementations with mocks Update Change Log
sagikazarmark
force-pushed
the
puli_discovery
branch
from
7f11469 to
2d3bc8d
Compare
As discussed #28, this PR aims to replace any custom discovery logic with Puli.
The convenient static wrappers are preserved, there is no major API change.
I am pretty in favour of this PR, really like how Puli works.
One arguable thing: this PR requires Puli to be installed on the user's machine or to be installed through composer (as in the require-dev of this package). I am fine with that requirement, even if it is beta, but I would like to hear other opinions about this.
Many thanks to @webmozart for helping me with Puli.