Re: [PEPr] Comment on SCM::SVN

On 4/17/04 5:23 PM Pacific Time, PEPr (pear-sys@php.net) wrote: > > Alan Knowles (http://pear.php.net/user/alan_k) has commented on the proposal > for SCM::SVN. Alan, thanks for taking the time to do a walkthrough of the code and post some comments related to the code itself! It's much appreciated. : ) > General Comments > ----------------------- > Name needs lengthing.. Versioning_Svn_client or something.. > SCM_SVN::init() - should be loadDriver or something. I'm flexible on this. For the time being, I'd prefer to wait and see how the naming RFC shakes out. The general trend seems to me to be "shorter is better so long as it's not too short". > SCM_SVN::init() not clear why fetchmode/stackret are not in the options array? The main reason is that the options array is primarily intended to be for the options that are parsed for building the svn client command-line switches. They're also set to default to SCM_SVN_FETCHMODE_ASSOC and PEAR_ERRORSTACK_PUSH respectively, and can be overridden if the developer using the package prefers another option. I was influenced towards this general technique by the DB package. At any rate, I'm re-working my PEAR_ErrorStack usage anyway, so the stackret values are coming out in the next revision. A helpful nudge from Greg Beaver and a bit more digging into the examples in the PEAR_ErrorStack documentation have led me to believe I've taken the wrong approach with PEAR_ErrorStack in the current release. Will be fixed for the pre-voting release! > usage example at top of SVN.php would be helpful. (probably move/copy i t from > factory) Good point! : ) > you can use @version@ for the version string - it gets replaced by the > installer. Thank you! Didn't know that. > Consider merging SCM_SVN_Common into SCM_SVN - I'm flexible on this as well -- again, I was following the example set by other factory-based packages such as DB and MDB2, both of which have a separate "common" class that acts as the parent to the subclasses. However, I agree that it's not entirely necessary for SCM_SVN. > SVN_ADD: > if (!is_array($this->options) || sizeof($this->options) < 1) { > could be simplified.. (empty arrays eval to false) > if (!is_array($this->options) || !$this->options) { > ** actually this looks like a standard piece of options checking... - perhaps > refactor into Common, or factory It's not -- some of the svn subclasses don't have required options, such as SVN_Cleanup and SVN_List. > Constructors for commands appear redundant (could this code go in factory?) It may be able to, yes. The main thing I'm kicking around (and would welcome suggestions on) with the constructors is how to best allow for all of the Subversion shortcut commands. I don't want to have an array mapping of the shortcuts to the main classes in SVN.php, since that would mean extending the class would no longer be as easy as dropping a new subcommand class file into the SVN/ directory ... As the mapping of shortcuts would have to be edited every time such a command was added. On the other hand, dropping an additional command class in the SVN/ directory would still work ... You just wouldn't be able to call it with a shortcut unless the array mapping of shortcuts to commands was modified. Are there any particular rules for PEAR packages regarding constructors? Some packages have them, some don't, and some packages have constructors that do nothing. Is there a preference or leaning from the group as a whole? > looks like the option building could be refactored > a) str_replace('_','-', $opt); > $options = array( > targets => value > config-dir => value > auto-props => boolean > svn_path => ignore > path => ignore > ) > .. then do a generic argument building? I am reluctant to do that for a few reasons. Some of the subcommand switches in Subversion are very similar (target vs targets), and many of the options don't apply to all the subcommands, and will generate errors from the command-line client if they're used. Currently, I filter out any unneeded commands and record an PEAR_ErrorStack 'notice' error about the inclusion of unnecessary commands as a way to help the developer implementing the class be more tidy about what options are passed to what subcommands, as I want to discourage the use of some options on a wholesale basis, since that could lead to unpredictable results. In other cases, such as SVN_Delete, I've deliberately used "del_path" instead of "path" as the option, just to make it less easy to accidentally delete a file (or entire project) by passing in an options array containing options you meant to use on another subcommand. All that said, the str_replace('_', '-', $opt) is a no-brainer! Thanks for pointing that out, I'll implement that for sure. : ) > you should really do escapeshellcmd inside the prepare statements - this means > the libraries are 'safe' to use on the web.... - without considerations.. How about a "safe mode" type of setting? Default to the use of escapeshellcmd, but allow it to be disabled if the developer implementing the package really doesn't want/need it. ? > - did I miss something - the code mentions an xml parser the return values for > things like list, but I could not see any examples anywhere.. Ah! I need to copy/paste the example that's above the factory class in SVN.php into the SVN_Log.php file. Thanks! Meanwhile, that's where the SVN_Log example is in the current version. For convenience, here it is: require_once 'SCM/SVN.php'; $options = array( 'url' => 'https://www.example.com/repos', 'path' => 'your_project', 'username' => 'your_login', 'password' => 'your_password', ); // Run a log command $svn = SCM_SVN::factory('log', $options); print_r($svn->run()); The XML parser is used to get an easy array out of the SVN log command, which is (unfortunately!) the only Subversion command that currently outputs XML. Alan, thanks again for taking the time to go through the package in such detail -- I will take your comments into consideration as I'm working on the next "review release" prior to submitting the package for voting. Regards, Clay -- Killersoft.com