[PEPr] +1 for PHP::Mailman
| From: | Till Klampaeckel | Date: | Sat, 17 Sep 2011 02:22:16 +0000 |
| Subject: | [PEPr] +1 for PHP::Mailman | ||
| References: | 1 | Groups: | php.pear.dev |
| Request: | Send a blank email to pear-dev+get-54515@lists.php.net to get a copy of this message | ||
Till Klampaeckel (http://pear.php.net/user/till) has voted +1 on the proposal for PHP::Mailman.
Proposal information:
http://pear.php.net/pepr/pepr-proposal-show.php?id=665
Vote information:
http://pear.php.net/pepr/pepr-vote-show.php?id=665&handle=till
This vote is conditional. The condition is:
What Christian and Ken said.
In addition:
1. Can you have an empty password on a mailman installation? If not, an exception should be thrown
-- IMHO the error needs to be explicit. In most of these cases is_string is nice, but kinda
meaningless. I'd check for empty() if something is required.
I'd also leave out a default (in __construct()) if something is required.
Same for admin url and listname. Asking the object for an error shouldn't be necessary since
these are the most basic requirements for the class to work.
If you disagree, please let me know why. Maybe I'm wrong, or don't know the setup well
enough.
2. I'd also like to emphasize the unittest thing.
The following are minors (aka nice to see in the next release, etc.):
3. Speaking of 'hasErrors()' 1 is not true and 0 is not false.
4. @version tags are mixed up.
5. I like set-methods with a fluent interface. E.g. "return $this;" -- makes the calls
chainable. If there's an error, just throw an exception. Makes the API easier and cleaner
(IMHO).
6. Your checks in setReq() are redundant -- if it wasn't an instance of HTTP_Request2, it would
never get so far since the type hint would throw it off.
7. List could be its own object Service_Mailman_List with methods to subscribe/unsubscribe, list
members, etc.. But that's just a suggestion not mandatory to make my vote +1.
Thanks again for proposing this code and thanks for incorporating my previous suggestions! :-)