Re: [PEPr] +1 for PHP::UML

From: Date: Wed, 30 Apr 2008 13:15:14 +0000
Subject: Re: [PEPr] +1 for PHP::UML
References: 1  Groups: php.pear.dev 
Request: Send a blank email to pear-dev+get-49945@lists.php.net to get a copy of this message
I don't quite agree with your comment, and here is why: > While the code itself looks good, Thank you > you really need to work on the docblocks > since most methods miss them. I would not say "most methods". The main class (PHP_UML), for example, has all its methods commented. You probably talk about PHP_UML_XMI_Factory2, which is a subclass of PHP_UML_Factory. Since its methods are 99% the same as PHP_UML_XMI_Factory1's, I did not comment them. What I would suggest to do, perhaps, is abstracting all methods in the parent class, and commenting only the latter. I could as well use an interface, as it is common practice in Java - where all interfaces are commented, but the implementation classes very few. > Your factory classes should be named PHP_UML_XMI_Factory_v1, not > PHP_UML_XMI_Factory1 and *2 as they are now. According to which rule ? I purposely added that number (the OMG's XMI version) at the end of the classname *without* any underscore, since the _ tends to become a package separator, although this is still not quite formulated in those words in the CS. In fact, the current CS - that I know well now - states that: "The PEAR class hierarchy is also reflected in the class name, each level of the hierarchy separated with a single underscore" But PHP_UML_XMI_Factory1 and PHP_UML_XMI_Factory2 belong to the same "level of hierarchy". They are two implementation classes of an abstract class (PHP_UML_XMI_Factory), and I do consider that those three classes should belong to the same package. > This should also be reflected in > the file layout. I don't even want to think of it. > Constructs like > <?php > else > throw new PHP_UML_Exception('File '.$filename.' does not exist.'); > ?> > are not allowed, you need {} around the block. Ok, I'll fix that one. Nevertheless, let me remind the CS to everybody: "You are strongly encouraged to always use curly braces." It does not say: "You must always use curly braces." However, if the PEAR community considers that this is an obligation, the relevant coding rule must be updated conformingly. In a more general manner, although I am aware of its utility, that PEAR coding standard, I'm afraid of it, is not far from being a nightmare to many developers, who might be used to different personal, or professional coding rules, as you know. As this is my first attempt to write a PEAR package, I spent a lot of time, correcting my own code to adapt it to the PEAR CS. I was particulary annoyed by spaces instead of tabs, and I find very upsetting stuff like "} else {", all on the same line. Yet I fully respected those "space and indentation rules", thanks to PHPCodeSniffer (otherwise, if I had had to check everything by myself, I think I would have given up trying to contribute from long). But please, leave some air to the developers. Enforcing documentation, E_STRICT-compatible code, file formats, is sound and sensible, and I will do my best to add more method docblocks to PHP_UML (esp. to the parser, which is complex) But putting a conditional vote on a long package, because of a forgotten curly brace, and of an incorrect class naming - which turns out to be, in fact, not incorrect, but fully justified - is a bit excessive, IMO. Baptiste Autin

« previous php.pear.dev (#49945) next »