Re: updated docs for Error_Raise

Greg Beaver wrote:

Hi Alan, Alan Knowles wrote:

I had a look through it yesterday. - a few comments...

thanks for taking the time

most of the error raising methods did not appear to have a message as a default argument. - while error codes are more flexible, messages make the code infinately easier to read..

actually, all of them don't, which is one of the main features. The message that describes an error is separated from the data that describes an error, making it possible for applications to handle the errors gracefully and even take automatic fixing action in some cases. This cannot be done with straight error messages.

This is a very high overhead for 80% of situations, - If you look at some of the more common practices in Gtk..gnu stuff they would normally do something like vsprintf( get_text('some error message with arg %s and another arg %s'), $args ); which would mean a full on version looking like.. - PEAR:raiseError( "file %s was not found in path %s", // beginners level.. ERR_NO, // reasonable quality essentail.. array( //options - if 3rd arg is array .. assume new format. 'args'=> array($file,$path), 'package'=>'MyPackage', 'callback'=>array($object,'method'), 'trigger' => E_WARN, 'actionType' => PEAR_ERROR_DIE, 'errorType' => 'warning' // default applied by error::warning() ) ); What I'm getting at is that using the first 2 arguments matching the current pear error, would be a considerable reduction in a) learning curve b) flexibility (in terms of same solution fitting more situations.) I think the current options, which you are right, is just for setting callbacks for an error, is pretty dumb. - as you probably want to define your own callbacks for errors (as your class does.)

I like your choice of infinite :) a man of my tastes. I am of the opinion that it is infinitely better coding practice to use comments to document code - not the error message. Compromising functionality for readability doesn't seem wise to me:

Theres a balance there, too many comments can make the code unreadable :)

<?php PEAR::raiseError("file $filename was not found in path $path"); // <-- worse // can't continue without this file <-- better Error_Raise::warning('MyPackage', MYPACKAGE_ERROR_FILE_NOT_FOUND, array('file' => $filename, 'path' => $path)); ?> Note that the end-user sees no change to pre-existing code.

This makes a complete disassociation between error definition and message.. - when you are writing the real message you have to cross reference the error location... to work out what the error was supposed to say.. - It's a penalty I am beginning to find in a number of places where I've abstracted stuff to far recently.. (look up 3 files to find what was variables/etc. to use..)

<?php $e = $package->doSomething(); echo $e->getMessage(); // this code still works unmodified ?> However, it is now also possible to do this: <?php $e = $anotherpackage->doSomething(); if ($e->getPackage() == 'MyPackage' && $e->getCode() == MYPACKAGE_ERROR_FILE_NOT_FOUND) { $errorinfo = $e->getUserInfo(); $file = $errorinfo['file']; if ($newpath = $savethedaypackage->search($file)) {

      // attempt to catch a typo
      $promptuser->ask("did you mean $file in $newpath?", 'answer');

} } ?> As you can see, this is a huge benefit over displaying "error: file blah was not found in path /wrong/path/with/small/tyop" I'm assuming that $savethedaypackage has all of the necessary security precautions that anything relying on user input would have, i.e. a list of safe paths that are OK to search in.

the calling standard for the new methods appears to be [notice|warning|fatal....]($package,$errorcode,$options) would it not be better to follow the existing pear error format - and just utilize the options 'officially' ||*PEAR_Error::PEAR_Error*| ($message ,$code , $mode, $options , $userinfo)|

I'm not sure you meant a connection between the $options in the Error_Raise example, and the PEAR_Error example, but if so, I'd like to clarify. notice($package, $errorcode, $options) $options is not correct, this is $args in the source, because it stands for error-specific arguments. This is where information that would normally be placed directly into the error message goes. This is what I think is a little confusing about doing this way - it is not natrually clear what that array is for.. -

PEAR::raiseError("file $file could not be copied to $dir"); Error_Raise::error('DirPackage', DIR_ERROR_COPY_FAILED, array('file' => $file, 'dir' => $dir)); This is in contrast to the options and userinfo passed to PEAR_Error PEAR_Error::PEAR_Error's $options specify a) either a callback or b) a php trigger_error E_USER_BLAH constant. $userinfo is a string that is pretty much never used.

these extra args seems like a bit waste.. - It looks a bit like PEAR_Error grew and grew beyhond it's design there...

Of course, I think the docs at http://www.chiaraquartet.net/apidoc do a better job of explaining the usage of the methods than this email, check there for more detailed stuff.

where options includes package/userinfo etc.. While I can see the use for putting package in the error, - it's not something that is essential 100% of the time - eg. on a small project, where you may only just consider using PEAR_Error over returning true/false/string for errors, there is some logic in the current design that infers the level at which you use pear error, follows your complexity level - eg. beginners start with PEAR::raiseError("xxxx"), - and eventuall work their way to PEAR::raiseError("XXX",ERROR_X,PEAR_...,array('package'=>'MyPackage'))

I can see the logic behind this approach - it is exactly what PEAR_Error currently uses. Experience (by this, I mean PEAR CVS), has shown that the most advanced projects have never worked their way past PEAR::raiseError('message'). I am inclined to believe that this is because it is in fact much too complex to require a message, code, return value, callback, package, AND error-specific information.

It's also as most projects (like a little library/timesheet project I'm doing, it's an overkill to get too bogged down in error handling situations that will never happen). But a generic handle all error callback will do.. - eg. call tech support (and pay me to find the bug:) This

is why I wrote Error_Raise. It only requires the code, package, and any optional error-specific information - all other information is implied, and would be redundant. I also would contend that it is not PEAR's goal to make it easy to write bad code, but instead easy to write good code.

It's not to make excellent code for end users - I'd be dreaming if i thought (me) and all the other users of pear wrote excellent code all the time.. - we have deadlines, deliverables and cash to get in.. - forcing unneccessary requirements on projects, that just need a reasonably simple, extendable error handler is an overkill.. I personally would

rather not see any beginner code in PEAR, only professional code. I know you didn't mean to imply this in your comments, but I do think that problems of current error handling have not been solved by making error codes optional. I don't think PEAR is a place for lazy people, but it is a great place for beginners exactly because of that fact. PEAR encourages and even in some cases requires good coding practice that will result in longer development time, but in turn greater longevity and stability of coding projects, which means less time wasted trying to work around kludges. I have been known to use true/false return values in a pinch. I've also now learned that taking any shortcut makes it really, really hard to extend and debug code, no matter how clever I might think it is at the time. Of course, I've never needed to kill a bug before, and very few of the programmers I know have ever experienced one, but I hear they can be most devilish ;)

what I'm getting at is that what you are attempting could be a great upgrade for PEAR_Error, but at present it has a high entry level cost.. The replacement of the third parameter with an options array - would be a highly flexible way to upgrade pear_Error, and offer potential for future changes (also easy to test for...) Regards Alan

Greg

-- Can you help out? Need Consulting Services or Know of a Job? http://www.akbkhome.com