Re: Re: services_libravatar caching

Will there be a revision with these changes for the merge request submitted at some point? Or was the response too demotivational? On Mon, Oct 29, 2012 at 8:06 AM, Christian Weiske <cweiske@cweiske.de>wrote: > Hello Stelian, > > > > Changes are now in my fork, please have a look. > > >> 3. Please move DriverCacheLite.php to Driver/CacheLite and rename > > >> the class accordingly. > > 1. I'm a bit disappointed that you don't know about the PEAR > Class-to-filename conventions. They are even publicly accepted via > PSR-0: > > $filename = str_replace($classname, '_', '/') . '.php' > > So Services_Libarvatar/Cache_Driver should be in > Services/Libravatar/Cache/Driver.php > > For including the files, you assume that the file with its full is in > include path: > > > require_once 'Services/Libravatar/Driver.php'; > > No __DIR__ and "../" at all. > > Btw, all classes in that package need to be prefixed with > Services_Libravatar - so > > 2. There should be no default driver set, or at least a "null" driver > that does no caching at all. Currently Cache_Lite is required; > something that I would not want at all. > > 3. protected $dnsCache has "@var mixed". Why mixed? Shouldn't all > drivers implement the Services_Libravatar_Cache_Driver interface > Or is null allowed? Replace "mixed" with > "Services_Libravatar_Cache_Driver" > > 4. "@param int $time - length in seconds". Please remove the "-", it's > not necessary and not used anywhere else. > > 5. "@return bool;" Please describe what the return value means, e.g. > "true if saving succeeded, false if not" > > 6. "@return null;" This needs to be "@return void", to indicate no > return value. You also do not need to add "return null;" to that > code; PHP does that automatically anyway. > > 7. "Class available since Release @package_version@" > This would always update the package version, which is not what we > want :) The cache driver is a new feature, so the next > Services_Libravatar version containing it will be 0.3.0. Set this > version hard in there. > > 8. I'd like to be able to change the settings of the Cache_Lite object. > So please either allow passing a Cache_Lite object to the Driver > constructor, or make a getCacheLite() method in that driver. > > > > -- > Regards/Mit freundlichen Grüßen > Christian Weiske > > -=≡ Geeking around in the name of science since 1982 ≡=- >