[PEPr] Comment on Console::Console_CommandLine

Travis Swicegood (http://pear.php.net/user/tswicegood) has commented on the proposal for Console::Console_CommandLine. Comment: 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. :-) Proposal information: http://pear.php.net/pepr/pepr-proposal-show.php?id=517 -- Sent by PEPr, the automatic proposal system at http://pear.php.net