Cache combined puli.json files of all installed packages by msojda · Pull Request #47 · puli/manager

msojda · 2016-01-10T15:13:56Z

tgalopin · 2016-01-10T16:48:26Z

src/Api/Cache/CacheFile.php

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.

webmozart · 2016-01-10T20:19:15Z

This is a good start already! :) Let's extract an interface PackageProvider out of PackageManager that contains all the methods related to fetching or querying packages. We can let CacheManager extend PackageProvider.

webmozart · 2016-01-29T11:20:42Z

Hi @msojda :) I refactored the JSON conversion a bit which should make it easier for you to implement this. Basically, you can implement JsonConverter in your own custom CacheFileConverter class and accept the JsonConverter for converting individual package files in the constructor (where we are going to inject the PackageFileConverter in Puli). This way, this should be fairly easy to solve.

webmozart · 2016-02-02T11:07:35Z

src/Cache/CacheFileConverter.php

targetVersion is not supported here, since this is not a MigratingConverter. You can simply store it in the latest version.

webmozart · 2016-02-02T11:09:36Z

This looks good already :) With some more work we should be able to finish this.

msojda · 2016-04-15T11:07:25Z

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.

webmozart · 2016-08-09T10:45:23Z

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 · 2016-08-09T12:48:32Z

Hi @webmozart. Just updated it with the latest repository changes. Ready to go for me! 😄

webmozart · 2016-08-09T13:19:05Z

src/Api/Container.php

    /**
     * Returns the configuration file manager.
     *
-     * @return ConfigFileManager The configuration file manager.


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.

Yeah, I just did that cause StyleCI was failing 😞

webmozart · 2016-08-09T13:34:28Z

That was fast! :) Thanks for the quick update. Apart from my remarks, the cached modules should actually be used in the Container class. I think they should be used wherever we now call ->getModuleManager->get*(). That means, one needs to explicitly query the module manager in order to work with the fresh modules, which should be fine.

msojda · 2016-08-09T13:41:38Z

I'll update the PR tonight. Shall I update the Container class as well or that's the thing for another PR?

PS. Will you be available on Gitter? We could prioritise next tasks then 😄

msojda · 2016-08-09T18:51:50Z

@webmozart I've done the refactoring regarding caching module list in CacheManagerImpl.

webmozart · 2016-08-10T13:46:05Z

Cool thanks :) Yes I think it would be good if you could update the Container class as well.

msojda · 2016-08-10T16:03:06Z

tests/Api/Fixtures/root/module1/puli.json

@@ -1,3 +1,4 @@
 {
+    "name": "vendor/module1",


@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?

Looks ok to me

msojda · 2016-08-10T16:03:41Z

Updated the PR @webmozart 😄

webmozart · 2016-08-11T10:58:51Z

src/Api/Cache/CacheFile.php

+     */
+    public function getModuleFile($moduleName)
+    {
+        Assert::ModuleName($moduleName);


Hey @webmozart - can't see any typo here..

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 ;)

webmozart · 2016-08-18T06:50:36Z

ping @msojda

webmozart

Looks good to me now apart from a minor issues!

webmozart · 2016-10-11T18:47:15Z

src/Api/Cache/CacheFile.php

+     */
+    public function getModuleFile($moduleName)
+    {
+        Assert::ModuleName($moduleName);


case typo here, should be moduleName(

webmozart · 2016-10-11T18:50:26Z

src/Cache/CacheManagerImpl.php

+        $path = $this->getContext()->getConfig()->get(Config::CACHE_FILE);
+        $path = Path::makeAbsolute($path, $this->rootDir);
+
+        if ($this->storage->fileExists($path) && false === $this->isRootModuleFileModified()) {


I'd prefer if you used ! over false === (also in line 133)

msojda · 2017-01-13T09:16:25Z

Hey @webmozart - requested changes have been done 😄

tgalopin reviewed Jan 10, 2016
View reviewed changes

webmozart reviewed Feb 2, 2016
View reviewed changes

msojda force-pushed the json-cache branch 4 times, most recently from fba432f to 0ec12b1 Compare February 9, 2016 12:24

msojda changed the title ~~[WIP] Cache combined puli.json files of all installed packages~~ Cache combined puli.json files of all installed packages Feb 9, 2016

msojda force-pushed the json-cache branch 2 times, most recently from b94e615 to d3dab6e Compare August 9, 2016 12:44

webmozart reviewed Aug 9, 2016
View reviewed changes

msojda force-pushed the json-cache branch from d3dab6e to b94e615 Compare August 9, 2016 17:40

Nyholm mentioned this pull request Aug 9, 2016

Do not remove dots in doc block #57

Merged

msojda force-pushed the json-cache branch from b94e615 to b42280a Compare August 9, 2016 18:49

msojda force-pushed the json-cache branch from b42280a to 77591bb Compare August 10, 2016 15:56

msojda reviewed Aug 10, 2016
View reviewed changes

webmozart reviewed Aug 11, 2016
View reviewed changes

msojda force-pushed the json-cache branch from 77591bb to dd251c9 Compare August 21, 2016 08:41

webmozart requested changes Oct 11, 2016

View reviewed changes

msojda force-pushed the json-cache branch from dd251c9 to 53c22d4 Compare January 12, 2017 21:33

Cache combined puli.json files of all installed packages

4255e22

msojda force-pushed the json-cache branch from 53c22d4 to 4255e22 Compare January 12, 2017 21:36

Comments

Conversation

msojda commented Jan 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webmozart commented Jan 10, 2016

Uh oh!

webmozart commented Jan 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webmozart commented Feb 2, 2016

Uh oh!

msojda commented Apr 15, 2016

Uh oh!

webmozart commented Aug 9, 2016

Uh oh!

msojda commented Aug 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webmozart commented Aug 9, 2016

Uh oh!

msojda commented Aug 9, 2016

Uh oh!

msojda commented Aug 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webmozart commented Aug 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msojda commented Aug 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webmozart commented Aug 18, 2016

Uh oh!

webmozart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msojda commented Jan 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

msojda commented Aug 9, 2016 •

edited

Loading