Skip to content

Cache combined puli.json files of all installed packages - #47

Open
msojda wants to merge 1 commit into
puli:masterfrom
msojda:json-cache
Open

Cache combined puli.json files of all installed packages#47
msojda wants to merge 1 commit into
puli:masterfrom
msojda:json-cache

Conversation

@msojda

Copy link
Copy Markdown

Comment threadsrc/Api/Cache/CacheFile.php Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

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.

Comment threadsrc/Cache/CacheFileConverter.php Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@webmozart

Copy link
Copy Markdown
Member

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

@msojda
msojdaforce-pushed the json-cache branch 4 times, most recently from fba432f to 0ec12b1CompareFebruary 9, 2016 12:24
@msojdamsojda changed the title [WIP] Cache combined puli.json files of all installed packagesCache combined puli.json files of all installed packagesFeb 9, 2016
@msojda

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown
Member

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
msojdaforce-pushed the json-cache branch 2 times, most recently from b94e615 to d3dab6eCompareAugust 9, 2016 12:44
@msojda

Copy link
Copy Markdown
Author

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

Comment threadsrc/Api/Container.php
/**
* Returns the configuration file manager.
*
* @return ConfigFileManager The configuration file manager.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I just did that cause StyleCI was failing 😞

@webmozart

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Author

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 😄

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@msojda@webmozart@tgalopin