Re: Re: [PEPr] -1 for Tools and Utilities::CodeGen_MySQL_UDF
| From: | Hartmut Holzgraefe | Date: | Thu, 01 Sep 2005 21:13:11 +0000 |
| Subject: | Re: Re: [PEPr] -1 for Tools and Utilities::CodeGen_MySQL_UDF | ||
| References: | 1 2 3 | Groups: | php.pear.dev |
| Request: | Send a blank email to pear-dev+get-39684@lists.php.net to get a copy of this message | ||
FYI: i've uploaded a new package to
http://hartmut.homeip.net/CodeGen_MySQL_UDF-0.9.1dev.tgz
there is no chagelog as i can't use PEAR CVS for this package yet,
but most of the changes should be listed below
Jesper Veggerby Hansen wrote:
I can appreciate the method names not being compliant, but I have other issues: 1) Indentation is messed up : don't use tabsfixed
2) Private members are not prefixed with _does this rule still apply to 5.0 code? (all members are now declared "protected" instead of "var") and if so: why has nobody raised this concern with CodeGen or CodeGen_PECL?
3) Inline phpdoc tags are missing some placesshould all be there by now
4) You have a dependency on PHP5: why not use the functionality it provides fx. in terms of access modifiers? You use the PHP5 functionality to some extent in your base class static methods, abstract classes. Why do you rely on the "documentation" way for access, i.e. '@access private', when you can do 'private funtion ...'?legacy code, work in progress, should now be fixed on all member variables but not necessarily all member functions (some are public but should be protected)
5) Maybe this just relates to 3 + 4: In Function.php there are 2 methods : cData() and cPrototype(), which have no phpdoc tags, no access modifier => public, then the names are not well chosen.fixed
"6") Is more to the process - it should ring a bell that nobody commented on the proposal, I think you ought to have "asked" the mailinglist for comments before voting (which is normally thought of as good practice). I (as some others) missed the proposal mail.what is an automated process good for if it still needs manual interaction? ;)
This is not (in itself) the reason why I voted -1 - the code probably works and all, I just think there are things that need to be changed before it is accepted, and I don't really think the community got a fair chance on giving input!getting input would be easier if the PEAR infrastructure (CVS, Package Pages, Installer) could already be used during the proposal and voting phases (maybe using a staging area insteat of the 'real' stuff), but thats a different story after all ...