Cache combined puli.json files of all installed packages #47
Conversation
| use Puli\Manager\Api\Package\PackageFile; | ||
| use Puli\Manager\Assert\Assert; | ||
|
|
||
| class CacheFile |
There was a problem hiding this comment.
I think you can add a small description about the class (and a author tag for you if you want :) ). In other classes as well.
|
This is a good start already! :) Let's extract an interface |
|
Hi @msojda :) I refactored the JSON conversion a bit which should make it easier for you to implement this. Basically, you can implement |
|
|
||
| foreach ($cacheFile->getModuleFiles() as $moduleFile) { | ||
| $jsonData->modules[] = $this->moduleFileConverter->toJson($moduleFile, array( | ||
| 'targetVersion' => $moduleFile->getVersion(), |
There was a problem hiding this comment.
targetVersion is not supported here, since this is not a MigratingConverter. You can simply store it in the latest version.
|
This looks good already :) With some more work we should be able to finish this. |
msojda
force-pushed
the
json-cache
branch
4 times, most recently
from
fba432f
to
0ec12b1
Compare
msojda
changed the title
|
Ping @tgalopin @webmozart :) PS. If this is good and we can progress with it I remember there was other issue we were talking about and it was related to this one. |
|
Hey @msojda! What's the current status and what is needed to finish this? This PR looks good and I'd like to merge it soon. |
msojda
force-pushed
the
json-cache
branch
2 times, most recently
from
b94e615
to
d3dab6e
Compare
|
Hi @webmozart. Just updated it with the latest repository changes. Ready to go for me! 😄 |
| @@ -625,7 +638,7 @@ public function getFactoryManager() | |||
| /** | |||
| * Returns the configuration file manager. | |||
| * | |||
| * @return ConfigFileManager The configuration file manager. | |||
There was a problem hiding this comment.
Apparently you changed all the doc blocks to remove the dots. On purpose?
While we can do this, I wouldn't do it in this PR.
There was a problem hiding this comment.
Yeah, I just did that cause StyleCI was failing 😞
|
That was fast! :) Thanks for the quick update. Apart from my remarks, the cached modules should actually be used in the |
|
I'll update the PR tonight. Shall I update the PS. Will you be available on Gitter? We could prioritise next tasks then 😄 |
|
@webmozart I've done the refactoring regarding caching module list in |
|
Cool thanks :) Yes I think it would be good if you could update the |
| @@ -1,3 +1,4 @@ | |||
| { | |||
| "name": "vendor/module1", | |||
There was a problem hiding this comment.
@webmozart Added this along with tests/Api/Fixtures/root/module2/puli.json:2 cause cache file serialization was failing because of missing names. Is that the behaviour what we want? Will we have any cases where names are missing? Should we handle it another way?
|
Updated the PR @webmozart 😄 |
| */ | ||
| public function getModuleFile($moduleName) | ||
| { | ||
| Assert::ModuleName($moduleName); |
There was a problem hiding this comment.
Hey @webmozart - can't see any typo here..
There was a problem hiding this comment.
I'm talking about the capital "M". I got the feeling you did a search-replace for "Package" → "Module" without checking the "case sensitive" checkbox ;)
|
ping @msojda |
| */ | ||
| public function getModuleFile($moduleName) | ||
| { | ||
| Assert::ModuleName($moduleName); |
There was a problem hiding this comment.
case typo here, should be moduleName(
| $path = $this->getContext()->getConfig()->get(Config::CACHE_FILE); | ||
| $path = Path::makeAbsolute($path, $this->rootDir); | ||
|
|
||
| if ($this->storage->fileExists($path) && false === $this->isRootModuleFileModified()) { |
There was a problem hiding this comment.
I'd prefer if you used ! over false === (also in line 133)
|
Hey @webmozart - requested changes have been done 😄 |
Fixes puli/issues#148.