Re: Services_Libravatar package in PEAR

Hello Melissa, > > I just saw that, although your package got accepted through voting, > > you didn't register the package yet. Also I found > > http://www.geekosophical.net/?p=535 > > > > I'd really like to see the package finished in PEAR. Did you give > > up? May I help you? Where? > I didn't register the package because as best I could tell, it only > got "accepted" due to a bug in pearweb at the time. PEPr does not have a bug; conditional votes count toward your total score. This is by design: Your package is considered worthy, but there are things that need to be changed before publishing it. > There's numerous conditional votes, includng many which together > essentially dictate refactoring the entire thing for no functional > gain, and one condition (using an additional package instead of a php > function) which I have no intention of doing because it's ridiculous. I've listed all the notes/conditions about your package in https://pastee.org/u7apk The ones that are left are: 1. Rename URL to getUrl() 2. no options array 3. unit tests 4. methods too long 5. use net_dns(2) 6. caching Let's look at these issues: 1+2: Rename url() to getUrl() / no options array These both belong togehter and are valid objections because currently, you are using the class only as a namespace for methods and don't use any of the benefits a object gives you. Also, url() is not something we like to see because we commonly prefix methods with get/set. The usecase of Services_Libravatar is to fetch avatar images for people, and that's often needed on blog comments and blog posts. Note the plural here. Currently, when fetching the avatar images for several people, one always has to pass the same options to the url method which makes no sense at all: I want to initialize/create the object with the correct options, and then fetch as many images as I need, without having to care about the options. The current code hinders abstraction in application code, because one cannot simply pass a avatar object to the app's comment rendering code, but always has to pass the options array as well. Thus using __construct() for the options is sensible to do. Further, allowing to change the options later on is also something that might be necessary in complex application code - so having setter methods for the options is a must, too. 3, 4, 5: unit tests, methods too long, net_dns2 usage They all belong together. First, try to write stable(!) unit tests for it that still run in 2 or 5 years without any change to them. You will experience that doing tests against actual live services will break earlier or later, but they will break because the user's avatar you're trying to retrieve changes or the URL changes because he chose to change his libravatar host. So you need prepared responses for DNS queries, which means that you need to add code for that crafted responses to your class, or you just use a dependency object that resolves DNS queries for you. In your tests, you can easily exchange that with one that gives you exactly the results. Using Net_DNS2 here is the logical solution because it gives you that ability already. Changing the libravatar's internal Net_DNS2 object in your tests is easy. It also allows you to add caching (#6) easily by wrapping the Net_DNS2 object in a cache object. While writing tests you will also note that it's hard to tests all code paths in your methods because they are too long, and do too many things at once. Splitting those up in multiple methods makes it so much easier to cover all the possible paths, and the code more understandable. So in my eyes, all of the comments and conditions have been made because they are valid and make sense. > The lib as it currently stands has been adopted by various other > projects already, including Moodle. Adoption is there because your code gives a working solution to a common problem. Making the code better isn't diametral to this. > I'd be happy for the existing version of the lib, as it is, to be > truly accepted to PEAR, but if we rewrote it, that may upset those > projects that have adopted the library. Why should it upset people when the code gets tests and is made better? Nothing hinders us to keep the old public "url" method around and deprecate it. This doesn't break anything. > So yes, I did essentially give up, as I haven't the spare time to > rewrite it. I'd like to make those changes. > I'm not sure how you can help me. As far as I'm concerned, there's a > perfectly functional library doing exactly what is needed, which the > Libravatar founder is entirely happy with and which complies with pear > standards and has been adopted in the wild. What needs doing is > awareness as to whether I should register it in PEAR or not, given > the conditional votes. Please register it. It can (and should) be released as alpha, and we can make the advised changes to get it stable, while keeping BC. -- Regards/Mit freundlichen Grüßen Christian Weiske -=≡ Geeking around in the name of science since 1982 ≡=-

Attachment: [application/pgp-signature] signature.asc