Re: [PEPr] +1 for Web Services::Services_GeoNames
| From: | David Jean Louis | Date: | Sat, 13 Dec 2008 11:46:02 +0000 |
| Subject: | Re: [PEPr] +1 for Web Services::Services_GeoNames | ||
| References: | 1 | Groups: | php.pear.dev |
| Request: | Send a blank email to pear-dev+get-51294@lists.php.net to get a copy of this message | ||
Hello Mike,
Michael Gauthier (http://pear.php.net/user/gauthierm) has voted +1 on the proposal for Web Services::Services_GeoNames. Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=550 Vote information: http://pear.php.net/pepr/pepr-vote-show.php?id=550&handle=gauthierm Comment: Looks good to me. I'm not certain it's a good idea to make your HTTP_Request2 object a public property. In my opinion, it would be better served as a protected property with a public setter method.why ? getters/setters are just evil imho ;)
Also, you should clone the request (use it like a template) rather than reusing. See the discussion on http://pear.php.net/bugs/bug.php?id=15072 for details. This is implemented in Services_Amazon_SQS as well.I'll have a look at this.
More specific exception types would be useful, as would documenting under which conditions exceptions are thrown in the @throws sections.Not sure what other exceptions the package should raise, what do you had in mind ?
I see you've already updated the code based on other people's suggestions, so that's all I've got :) I like how you're including the response object in exceptions caused by HTTP. I'll have to add that to my packages.Thanks for your comments. -- David