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

1, Fair comment, applied. 2, The "i" modifier, means ignore case, so HTML/html/hTmL should all work. If the string contains <HMTL> for all intents and purposes, it probably is HTML. It is just a basic check. 3, It's HTML, not XML. I have good reason to use regex over domdocument, but I've yet to explore Xpath. I feel a new version coming on... 4, Fair point. Fixed. 5, "Package-specific exception class is required" -- can you show me an example? 6, This is getting ridiculous. The PHP manual shows returning values unless its an error so I'm sticking with that... 7, It already has a package.xml 8, It has unit tests, although I should implement a few more... [later] 9, Until we do wide scale testing, we can't begin to predict which versions work and which don't. Implementing limitations based on a version number is illogical, instead it should be feature based. A bit like how modernizr detects what features your browser has rather than relying on the version sent. Thanks for the tips. On Wed, Jan 18, 2012 at 10:30 PM, Michael Gauthier <mike@silverorange.com>wrote: > Michael Gauthier (http://pear.php.net/user/gauthierm) has voted +1 on the > proposal for Web Services::Mailman. > > Proposal information: > http://pear.php.net/pepr/pepr-proposal-show.php?id=672 > Vote information: > > http://pear.php.net/pepr/pepr-vote-show.php?id=672&handle=gauthierm > > This vote is conditional. The condition is: > > 1.) Clean up variable and method names. Don't need to shorten request to > req, it just makes it harder to read. $adminurl should be $adminURL. > > 2.) in fetch(), the check to see if it is HTML could be made more robust. > What is the service changes to using lower case <HTML>? What if the > response is not HTML but contains the string <HTML>? > > 3.) Use DOM+XPath to parse responses rather than regular expressions. > Using regular expressions to parse XML is questionable. > > 4.) in fetch, you throw an exception and then return false. The return > false will never execute. This antipattern is repeated in other places in > the package. > > > 5.) Package-specific exception class is required. Right now it's not > possible to handle errors from this package. > > 6.) Your methods should have no return and always throw exceptions rather > than having to both catch expected exceptions and check for false as a > return value. > > 7.) needs a package.xml > > 8.) Needs unit tests. > > 9.) Since the code looks heavily dependent on the output of a particular > mailman version, perhaps there should be some sort of support for > versioning and/or mailman version detection. >