Published 2004-09-23 22:48:48

I've been having a little attempt revamping my subversion viewer, seeing if I can
  1. Integrate it into the new site
  2. Use VersionControl_SVN, rather than the quick hack I wrote, Subversion_Stream.
I have to admit, though, the API for VersionControl is not exacly clean and user friendly. This is a general overview of the gripes.

  • Uses PEAR_ErrorStack, which hides errors, even worse than PEAR_Error used to.. DataObjects uses a ErrorStack style system, you can print_r the dataobject, and it shows you the last error as a propery of the object... (simple).

    Error stack seems to insist on you loading it, then only using it's methods to check stuff. - the actually error is hidden away in the stack object..

    Trouble is, that a number of the functions in DataObjects needed to return true/false (fetch, for example), so returning an PEAR_Error, was problematic then (in hindsight, alot of the other methods should have returned PEAR_Error, but that broke BC - and in the future it should throw exceptions).
    So, DataObjects had some logical justification for messing with the standard a little, VersionControl_SVN hasnt really, and it just adds a dependancy on a eventually redundant package.

  • Trying to make things easy ending up with a messier API,

    $x = VersionControl_SVN::factory(array('list','cat'), $options);

    while this is cute, it adds an horrific amount of complexity to the API, and only saves you a little bit of typing, at the expense of clarity and readibility.

  • Counterintuative program flow.

  • class VC_SVN {
    .... function run() {
    ....... $this->prepare();
    ....... exec in here..
    ....... $this->parseOutput();

    VC_SVN_List extends VC_SVN {
    ..... function prepare() {

    So you call run on the base class, and the body of what happens is in the extended class. - so if you look at the 'action' classes, it is impossible to follow the flow of the application, without having a perfect memory of the base class..

    it would have been far better to have a run method in each action class, and call some generic methods from the base class.

  • direct API mapping makes it a little unclear how it should work:
    example -

    require: svn list -v {mysite}

    $svn->list->run(array({mysite}), array('v'=>true));

    (and if you get it wrong, not much happens)
    ok - how is this simpler than:
    $svn->run('list -v ', $mysite);

Mentioned By: : VersionControl_SVN ( referals) : december ( referals) : april ( referals) : php VersionControl_SVN ( referals) : php exec svn ( referals) : PEAR_ErrorStack ( referals) : PEAR_ErrorStack example ( referals) : VersionControl_SVN examples ( referals) : using VersionControl_SVN ( referals) : how to use VersionControl_SVN ( referals) : versioncontrol_svn php ( referals) : pear_errorstack examples ( referals) : exec svn php ( referals) : pear VersionControl_SVN ( referals) : php exec "svn list" ( referals) : php exec svn list ( referals) : using PEAR_ErrorStack ( referals) : VersionControl_SVN.php ( referals) : VersionControl_SVN::factory ( referals) : using pear_Error ( referals)


Re: VersionControl_SVN review

Thank you for the critique! I hope that aside from the above gripes, you found the package useful otherwise.

Regarding the issues you've raised ...

<li><b>Use of PEAR_ErrorStack</b></b>
While I was writing VC_SVN, the following post about PEAR_ErrorStack popped up on the PEAR-DEV list.

Not wanting to waste time using PEAR_Error if the new standard is PEAR_ErrorStack, I just used PEAR_ErrorStack.

The raging debate on PEAR-DEV regaring error handling, exceptions, etc. has far exceeded the amount of time I have to spend reading list posts ... so until something formal is posted on about a final decision on error handling in PEAR packages, I still believe that the choice of PEAR_ErrorStack was the best choice for VersionControl_SVN, and will most likely be my choice for future packages.</li>

<li><b>Making things easy vs. Messy API</b>
The snippet in your example ...

$x = VersionControl_SVN::factory(array('list','cat'), $options);

... is an optional way to do things. This would work just as well:

$svnlist = VersionControl_SVN::factory('list', $options);

$svncat = VersionControl_SVN::factory('cat', $options);

The option of a multi-command factory is to save some keystrokes for those instances when you need to use a number of svn subcommands. But, it <b>is</b> an option, not a requirement ... as stated pretty clearly (I think, anyway) in the documentation.</li>

<li><b>Counterintuitive Program Flow</b>
The run() method doesn't change between subcommands, but the prepare() and parseOutput() methods do. In trying to keep the package small and maintainable, it made sense to me to keep the code that's the same through out the package in the base class.

I'm open to suggestions on how to approach this differently ... but naturally I'm looking for ways to keep the package as light as possible.</li>

Alan, I definitely do value your comments -- if you recall, I made numerous tweaks to the package based on your initial review when VC_SVN was in the propsal phase. I guess I don't agree with the issues you've raised here, but your previous comments definitely made VC_SVN a much better package than it was originally.
#0 - Clay Loveless ( Link) on 2004-09-24 00:46:51 Delete Comment

Add Your Comment

Follow us on