Re: [PEAR-DEV] 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 <[email protected]>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 ≡=-
>