Re: [PEPr] +1 for Web Services::Services_UseKetchup

On Thu, Sep 9, 2010 at 1:36 AM, Daniel O'Connor <daniel.oconnor@gmail.com> wrote: > > Daniel O'Connor (http://pear.php.net/user/doconnor) has voted +1 on the proposal for Web > Services::Services_UseKetchup. > > Proposal information: > http://pear.php.net/pepr/pepr-proposal-show.php?id=628 > Vote information: > > http://pear.php.net/pepr/pepr-vote-show.php?id=628&handle=doconnor > > This vote is conditional. The condition is: > > Bunch of trivial nitpicks; sorry for not commenting earlier! No problem. Appreciate it none the less. :-) > I know you've done it to save you writing code, but work in the constructor scares me a > touch :S - $this->getApiToken(); The token is necessary though - think of it as a frob (Flickr) or poor man's oauth. ;-) I've moved the call: http://github.com/till/Services_UseKetchup/commit/6f20082929d40b24da2e7de9cbace0ac8748498e I decided to keep those methods in that class because I rather configure all other objects with it, then e.g. requesting the token again and again when I use the different objects. > > Ditto instantiating classes in places that aren't the constructor - makeRequest() for > instance. I'm using an acceptor pattern: Services_UseKetchup_Common::accept(). I think offering a setter for the object allows overriding it so this shouldn't be an issue anymore. Please let me know. > > Also having some of the unit tests incomplete because a certain user account doesn't work > :( Don't understand your "question". I just ran the tests again: till@till-laptop:~/Services_UseKetchup$ phpunit --colors tests/AllTests.php PHPUnit 3.4.15 by Sebastian Bergmann. .............I Time: 14 seconds, Memory: 10.50Mb OK, but incomplete or skipped tests! Tests: 14, Assertions: 39, Incomplete: 1. The dist is an example. Rename it and put in your credentials. Of course it requires a valid account - I take it you either don't have one or it doesn't work? ;-) > PHPUnit's getMock() or HTTP_Request2's Mock Adapter ftw. Ah, yeah. Good idea. Creating those is a bit tedious, I'll make sure to put that in before 1.0.0. > From an overall API perspective; it might be worth splitting up the "create me a bunch of > classes, wire them together with these credentials" behaviour into a different class > (Services_UseKetchup_Factory) from the "execute these API calls and give me data" code in > the core class. Do you want me to move Services_UseKetchup::__get() to a separate class? I see no real reason for that. But let me know what exactly you're after. > I can see you've sort of done it with Services_UseKetchup_Common; but I'd encourage > you to really define the purpose of each class, and polish its responsibilities. Services_UseKetch_Common just hosts most of the request stuff and some getter/setter methods. Nothing fancy. It just made sense to not do this again and again. > > Look forward to seeing it in PEAR. > Thanks for your input! Till