David Coallier [2007-04-28 17:22 UTC] Could you please provide links to the php source please ?
Martin Jansen [2007-05-06 09:12 UTC] Can you put .phps files online somewhere? This way it is easier to browse through your soure code. Thanks!
Joshua Eichorn [2007-05-07 17:00 UTC] I would choose a different license then the PHP license, its not a great choice for code thats not part of the php engine.
Also I think you should look at refactoring the code a bit. Classes that are a couple thousand lines are really a pain to maintain, PhpDocumentor thought me that :-(
I would suggest moving the command line interface into another class
You might also want to think about a different name to make it clear what the class is doing, PHP_GenDocBlock maybe
Michel Corne [2007-05-11 19:50 UTC] Thanks for your useful comments.
I changed the package/main class name to GenDocBlock, split the main class into several smaller manageable classes, and changed the license to BSD. The links to the package source and doc are in the links section in this proposal main page.
Looking forwards your feedback.
Bertrand Mansion [2007-05-15 07:24 UTC] Looks like a very useful package.
It's well written and well documented and it fills a real need for a documentation generator. I suggest that you find a more explicit name. Maybe a name like PHP_DocBlock_Generator would do. I'd also rename the run() method to generate(). With a name like this, it will be found more easily via search engines.
I'd also be interested in seeing your other package about Unicode :)
Arnaud Limbourg [2007-05-15 07:56 UTC] The change of name suggested by bertrand makes sense since there is a PHP_Parser_DocblockParser, having PHP_Docblock_Generator makes sense, even PHP_Docblock (how likely is that there is going to be anything else to do with docblock besides providing an API to use DocblockParser ?).
Christian Weiske [2007-05-15 08:02 UTC] class PHP_GenDocBlock:
- newline between class definition and { (in all files)
- you are hiding errors when using file_get_contents - maybe you should have a better error handling mechanism
PHP_GenDocBlockTest:
- comment for run() method have strange indentation
same for PHP_GenDocBlock_Cli
PHP_GenDocBlock_Cli:
- You should either use Console_GetArgs or Console_GetOpts for parameter handling, not do everything yourself
PHP_GenDocBlock_Tokens:
- Strange indentation in comment for "public $eol;"
- process is unreadable, please insert a line feed after each break;
PHP_GenDocBlock_Type:
- Please try to stick to the 80 chars-a-line soft limit, e.g. when having comments after code (i.e. in extract()) - you should move the comment above the code
from:
if (($key = array_search('', $types)) !== false) unset($types[$key]); // removes the empty type/string
to:
// removes the empty type/string
if (($key = array_search('', $types)) !== false) unset($types[$key]);
- Why do you have "// /" in your array definitions?
private $license = array(// /
- Rethink if you need all variables private, or may make some of them protected to allow subclassing
- It is not docblock related, but maybe you want to add the cmdline switch to add the vim indentation settings to each file's bottom.
Joshua Eichorn [2007-05-15 16:29 UTC] PHP_Docblock_Generator would be fine with me. I just want a name that lets you know what the package does.
I would stay away from PHP_Docblock since that sounds to me like a class that lets you get docblock information at runtime (which could be implemented on top of reflection)
I haven't had a chance to review the code yet, but i'm glad to see you've broken things out into multiple classes.
Travis Swicegood [2007-05-15 21:46 UTC] I've given it a quick glance and it looks like a pretty useful package. There's some CS things to be addressed, but other commentators have already left notes on those. I would recommend checking out PHP_Beautifier package as it'll help you get pretty close automatically.
One thing I did see that I personally wouldn't do is the use of "and" and "or" to chain function calls together. For example, I would change:
[code]
// PHP_GenDocBlock::run()
$outfile = $outfile or $infile;
[/code]
To:
[code]
if (empty($outfile)) {
$outfile = $infile;
}
[/code]
To me, the latter reads more easily, but that's just a personal preference more than anything. In a repository like PEAR, the goal with my code is to make it as readable as possible to someone who's never seen PHP before. The original code I cited above uses lesser known conventions in PHP, so that'd be the main motivation for me to make the change. I would also rename gendocblock to php_gendocblock to avoid any possible naming conflicts and insure that's noticed on the system as a PHP related executable.
The only other thing I noticed is that you can't specify a docblock template. To me it would be extremely useful to be able to run:
[code]
$ cat MyGreatCode.php | gendocblock --template-path /path/to/templates
[/code]
I'm not certain what the template files would look like. Personally, I would just make it a very simple PHP file along the lines of this as the /path/to/templates/class.php file:
[code]
/**
* <?php echo $short_desc; ?>
*
* <?php echo $long_desc; ?>
*
* @author <?php echo $author; ?>
... etc., etc.
[/code]
Then I could specify my own docblock style (for example: skip lines between all tags, add license text to file level docblocks, etc.) in an external template file.
All-in-all, this seems to be a very useful package. One thing I am curious about is the naming of the package. I'm thinking Docs_DocBlock_Generator would be a good name. As you might have noticed on pear-dev, I am trying to figure out how to name another doc generator - this one for TestDox. As there seems to be a lot of documentation related packages right now, maybe it's time to add a Docs sub-category to Tools and Utilities.
Michel Corne [2007-05-18 11:15 UTC] Thanks for your comments. I tried to address them all.
pear-dev => "...name like PHP_DocBlock_Generator..."
MC => Sounds good. It should probably read PHP_DocBlockGenerator actually without the underscore in-between since underscores are meant to reflect a hierarchy level as I understand. Thoughts?
pear-dev => "...rename the run() method to generate()..."
MC => Fine with me. Will do in the next release.
pear-dev => "...newline between class definition and {..."
MC => Ah! I (re)checked the Pear Coding Standards and I could not find any mention of this as a requirement. I am using the PHPEdit Beautifier with the Pear settings which does not insert a newline accordingly! I did notice though that Pear packages have classes with the newline between the class definition and {. PHP_Beautifier (nice tool btw) does that but it screws up my arrays (see comments below)! So I guess I could do that manually before I commit the last change in a file. How important is that anyway? (just asking)
pear-dev => "...hiding errors when using file_get_contents - maybe you should have a better error handling mechanism..."
MC => Maybe. This package is really meant for developpers not for end-users. It is also mainly meant to be used with the shell command. Simply returning FALSE if files cannot be read or written seems good and simple enough to me at this point. I agree this could become a future enhancement though.
pear-dev => "...comment ... have strange indentation..."
MC => Oops! Forgot to (re)beautify a couple of files. Will do in the next release.
pear-dev => "...use Console_GetArgs or Console_GetOpts for parameter handling..."
MC => Right! I realized too late these packages were available after I coded/tested my stuff. I could use one of the other in a future release, definitely. Which one is recommended?
pear-dev => "...unreadable, please insert a line feed after each break..."
MC => That is kinda hard statement :-) Anyway, some "case's" go in the same block/break, so inserting a linefeed after those break won't hurt. Will do in the next release.
pear-dev => "...stick to the 80 chars-a-line soft limit ... move the comment above the code..."
MC => I would like to challenge you this one. The Pear Coding Standards says " It is recommended to keep lines at approximately 75-85 characters long for better code readability." First, this is a recommendation, not a requirement. Second, this made a lot of sense in those days when screens were small and people had to print their code to read it more easily. Nowadays editors and screens allow easy reading of long lines. I personaly comment each line of code for easier support/maintenance. This often makes 120 characters or so lines. I would like to stick to that.
pear-dev => "...Why do you have "// /" in your array definitions?..."
MC => Well, let me explain. Some small arrays fit in one line. Others are larger and easier to read if there is one line per element. PHP-Beautify merges all lines in one single line which is not good for large arrays. The ArrayNested filter expands all arrays over several lines including small ones which is not good either. The PHPEdit beautifier always merges the first element with the array() statement which is not always wanted. Adding /// does the trick and fools the beautifier; that is all I found! I guess it does not really hurt in the code. Any suggestion to fix this is welcome, really.
pear-dev => "...Rethink if you need all variables private, or may make some of them protected to allow subclassing..."
MC => Why not. I tend to make private those variables/methods that I do not need as public or protected as general rule. Anyway, I could look at changing some to protected in a future release based on actual users needs.
pear-dev => "...add the cmdline switch to add the vim indentation settings..."
MC => Good point. Could do in the next release. What should I add at the bottom of the line? I also saw "/* vim: set expandtab tabstop=4 shiftwidth=4 softtabstop=4: */" at the begining of some files. Is this different?
pear-dev => "...wouldn't ... use ... "and" and "or" to chain function calls together..."
MC => I guess we all have different styles to write code. I actually believe that the use of "and" and "or" may make the code lighter and easier to read.
pear-dev => "...can't specify a docblock template..."
MC => Good point. I also thought of a similar enhancement, e.g. using the page-level docblock of the main class. Others may have other ideas?
Philippe Jausions [2007-05-18 14:27 UTC] My 2 cents. The package should be filed under the Tools category as PhpDocBlockGenerator (whereas The PHP_DocBlock_Parser is meant as a package to be used in user code, not as a stand alone tool.)
A short name alias can always be created on the file system.
|
