Re: [RfC] Services_Weather

Hey Greg, I was waiting for your mail with anticipation :) Greg Beaver wrote: > Hi, > > Just to clarify: Unless your package is handling errors from another > package, I don't think it should be necessary to control error handling, > as there are many situations in which your package will be used: > > - debugging (catch and display all errors) > - production (suppress or re-format error messages for users) > - as part of another package (repackage errors into errors for the > package in some cases) Yes, I was a bit torn over that issue as well, but in fact I am handling or at least mutating certain errors received by XML_Unserializer and Cache, because in the first case I don't want to return its errors as they might a) confuse the user and b) might result from for totally different reason, not originating in the Unserializer at all. For the Cache, it has no usable Error handling (just look at the constructor), so I wanted to do that myself. > This is not a standard or even a guideline right now, but once all the > dust settles on the other things (BC, subpackages), I will probably > bring it up more formally so that the path of continual improvement is > walked :). > > Maybe change the call of raiseError() to: > > return PEAR::raiseError($message, $code, null, null, > "Services_Weather_Error", null, false); > > Then the constructor of PEAR_Error can use the global error handling > values. Hm. To be honest, I'm not totally aware of everything that occured in that error discussion, could you point me to the thread? If there is a common guideline, I'll adapt to it asap... > As for your package, I'm excited by what it can do, and the code's quality. > > It might make sense to name the weather.com error codes > > SERVICE_WEATHER_ERROR_WCOM_* or something to distinguish from package > errors programmatically, but I don't think that is a big deal. I'm not using the errors exclusively for weather.com, NO_LOCATION makes sense in Metar, too. > I would suggest naming checkData() Services_Weather_checkData() in > buildMetarDB.php, or explicitly specifying in a comment at the top that > this may cause problems if used via include() Done. > In the constructor for Services_Weather_Common, I would suggest the > better way to determine the presence of Science_Astronomy is to simply > iterate through the include_path, postfixing "/Science/Astronomy.php" > and using good 'ol file_exists() - the overhead will be much smaller, > and possibly more accurate, as there can be many PEAR installations, and > the only way to determine which you're in is to know before you start by > using a <replace /> value in package.xml - which is a bit too complex > for this package, I think. This sounds like a task for PEAR itsself to provide for all packages. Maybe I should remove the reference to Astronomy for now, this package needs some seasoning and I won't release that one in the very near future (read: next weeks). > Hope this is helpful :) It was. Best regards, Alex