Greg Beaver [2007-11-11 19:55 UTC] very interesting work, I'd love to hear what the various Console_* authors think, I need a tool like this for Pyrus's command-line frontend.
Some thoughts:
Your file-level doc comment doesn't yet conform to PEAR's CS, take a look at the manual, it's a simple fix (you need license plus PHP version and a few other minor details)
You should consider validating the XML using either RNG or XSchema with DOM's validation facility prior to parsing, otherwise you will end up with a whole host of possible odd bugs when users use invalid XML (and they will, trust me - years of experience with PEAR's package.xml has taught me this one :)
David Jean Louis [2007-11-11 22:22 UTC] Greg,
I'll check the manual and fix the file level doc, thanks for pointing this.
For the xml validation, in fact it's a todo (in the CommandLine/XmlParser.php file), I thought of validating xml against a dtd file, but I'll also check the RNG and XShema options, not sure what's better...
Cheers.
Travis Swicegood [2007-11-16 00:07 UTC] David, excellent idea! I used optparse for the first time a few weeks ago and had made a mental note that if I ever had the time I should put a PHP version together... you've just saved me the trouble :-) Now, on to an actual review of the code.
The biggest thing that jumps out at me is the name. Is Console_CommandLine the best name for this? When I first saw the proposal my mind went to some sort of front-end controller for CLI programs; something that you would use to build CLI program that gathered input from the user and such. The most obvious name change that comes to mind for me is Console_OptArgParser since it parses both.
One thing I would shy away from is the direct usage of fwrite() to print output. I would move to an Outputter class that could be injected at instantiation. That allows for custom output to be recorded in addition to or in place of fwrite. The use case for this that comes to mind is creating a ToFileOutputter that logs everything for sending back to a developer for debugging.
In the Renderer class I would remove the render prefix from the method names. Seems redundant to me, but that's 100% a style thing on my part. Having a method name that describes the action is great, but renderer->renderVersion() doesn't read as well as renderer->version() to me.
Why are there so many protected methods within the Renderer class? The only one that I would put as protected/private is the columnWrap() method. Everything else easily could be usable as part of the renderer itself. I'm not clear on the motivation of the double foreach() in most of those methods.
On the main class, the number of public properties is too high for my tastes. I would move them to protected/private and add accessors via __get()/__set() so basic type checking and such can be performed. I'm not clear on why Console_CommandLine::$errors is a static property.
I would also move the conditional require_once in the constructor. In that case I would make Renderer an interface with the various required public API, then make your default implementation something along the lines of Renderer_PlainTextRenderer.
On the callback type in the Option code, I would move over to call_user_func() so you could provide any type of valid callback pseudo-type.
I would also move dispatchAction() over to a traditional Strategy implementation so that it delegates the actual implementation of a counter, calling the callback, etc., to other objects, such as Console_CommandLine_Action_CounterAction, Console_CommandLine_Action_StoreIntAction, etc., etc. That allows for custom types to be added without much fuss. They would also be a good place to put type specific validation to help trim down CommandLine_Option::validate().
I'm not sure I see the reasoning behind the various static properties within the Option class either.
One final note, why are there trigger_error() calls in validate instead of throwing an Exception?
This is an excellent base to work from. I look forward to getting it so I can use and abuse it in my CLI apps. :-)
David Jean Louis [2007-11-16 08:50 UTC] Hello Travis,
wow, very nice code review !
I'll try to answer the best I can to your remarks:
>> The biggest thing that jumps out at me is the name.
>> Is Console_CommandLine the best name for this?
fair enough, in fact the library was initially called Console_OptionParser (as the main class of optparse python module) but I though it was to restrictive because Console_CommandLine handles options but also arguments and sub-commands - in other words a whole "command line". OptArgParser doesn't sound very good to me, maybe CommandLineParser would be more appropriate ? Need thinking...
>> I would move to an Outputter class that could be injected at instantiation.
very good idea, I'll implement it asap :)
>> In the Renderer class I would remove the render prefix...
another good idea, +1
>> Why are there so many protected methods within the Renderer class?
you're right they should be public, I'll fix this, thanks.
For the double foreach, it's because we have to calculate the max length of text before calling columnWrap() method, but maybe it could be done better, I'll look at this.
>> [...] the number of public properties is too high for my tastes.
>> I would move them to protected/private and add accessors via __get()/__set()
hmm, not sure about the magic methods idea, I'll think about it and keep you informed
>> I would also move dispatchAction() [...]
yep, good idea ! the possibility of defining custom types is definitively cool.
>> I'm not sure I see the reasoning behind the various static properties [...]
I put these static properties to keep messages and regexes in the same place, rather than having to search them in the code each time a change has to be done, static because they are the same in all instances, I'm opened to suggestions if it could be done in a more elegant way :)
>> why are there trigger_error() calls in validate instead of throwing an Exception?
Because I think that programming errors should just trigger_error and user errors (like bad usage of the program) should raise Exceptions, so that the programmer can display the error message to users with getMessage(), you think it would better to treat all errors as exceptions (with different execptions depending on the type of error, ala python) ?
>> This is an excellent base to work from.
And this was a very useful review, thanks !
David Jean Louis [2007-11-16 09:28 UTC] Travis,
sorry I forgot to answer to two of your remarks:
>> I would also move the conditional require_once in the constructor. [...]
I understand the renderer Interface suggestion, and I think it's a great idea, but why you want to remove the require_once, it is necessary to have a default renderer, no ?
Also, I would name the default renderer Renderer_Default (ala Quickform...) instead of Renderer_PlainTextRenderer as you suggested.
>> [...] I would move over to call_user_func() [...]
Sure, that's better! I'll fix this, thanks.
Bye.
David Jean Louis [2007-11-16 10:13 UTC] I answer to my last comment:
>> but why you want to remove the require_once, it is necessary to have a default renderer, no ?
Ahhhh, I understand, what you are suggesting is not to remove the require_once, but on the contrary to make it *not* conditional by calling a renderer factory that will find the corresponding renderer or the default renderer, right ?
Bertrand Mansion [2007-11-16 10:21 UTC] I don't see what this package does that Console_GetArgs cannot do already, except maybe the xml configurations for handwritten xml lovers.
I wish PEAR contributors were more "creative" than this, but since I don't care much about PEAR in general anymore, good luck with your proposal.
David Jean Louis [2007-11-16 11:12 UTC] Hello Bertrand,
as I said in the preliminary note, this package is very similar to Console_GetArgs and I won't push for it if Console_GetArgs is enough for everybody ;)
But let me point some things it can do that Console_GetArgs cannot:
* Sub commands, AFAIK Console_GetArgs does not handle things like:
$ <yourprogram> -q install -v <something>,
* Customization, again, AFAIK generated help is not customizable with Console_GetArgs,
* Complex options checking, when you want a program to handle options orders for example or options exclusion (<yourprogram> -f<file> cannot be mixed with another option for example), with Console_CommandLine you can always use callback to do the hard stuff,
* And as you pointed, xml definitions, not that I'm a handwritten xml lover eh, but it can be useful for big projects or when you have several frontends for a program.
To be honest I tried to extend Console_GetArgs at first but didn't manage to implement what I wanted without breaking it all.
Cheers.
[off topic: your comment is a bit harsh, I don't know/care why such rancor, but again: I won't cry if the package is not integrated into PEAR, I just thought it was a good idea to propose it since it was yet developed and could be useful to others... As for not being creative, please don't worry too much for me ;)]
David Jean Louis [2007-12-01 14:58 UTC] Hi all,
since submission of Console_CommandLine, I made all the changes suggested that I considered relevant/feasible and added a user manual (work in progress).
All stuff is available here:
http://code.google.com/p/console-commandline/
As it is my first proposal, I prefer asking:
should I set the package to "call for votes" or there's just not enough interest for this package ?
Thanks.
|
