On Thu, Oct 25, 2012 at 04:25:46PM +0100, Dato Simó wrote:
> I see this has been LGTM'd by Guido already, these are some minor
> questions that I had about the first half of the patch, of which I had
> completed review.
>
> On Fri, Oct 12, 2012 at 15:20 +0200, Iustin Pop <[email protected]> wrote:
>
> > + , ("ConfigVersionMismatch", [ ("expCode", [t| Int |])
> > + , ("actCode", [t| Int |])])
>
> Not 'expVer' and 'actVer'?
You know I'm not good at names :)
I'm fine either way. Or you can send a patch, this is already committed.
> > + , ("ParameterError", [excErrMsg])
>
> How closely (a) is this file meant to follow the Python exceptions, and
> (b) Python exception docstring describe actual use?
>
> The Python docstring for ParameterError says "The argument to this
> exception should be the parameter name", though I see plenty of uses
> where the argument is a full-formed message error. (So I guess
> ParameterError can stay with excErrMsg.)
See previous discussion on this topic; the problem is that the Python
exceptions are not consistent, so in these cases I'm using just a
string.
> > + , ("OpPrereqError", [excErrMsg, ("errCode", [t| ErrorCode |])])
>
> Does errCode need to be marked optional (or an UNKNOWN value added to
> the ErrorCode ADT)? Python docstring indicates it needn't be specified.
Sadly that's out of date. For OpPrereq, we have committed to our
external interface that an error code will be present. I guess the
python docstring needs to be updated.
> > + , ("X509CertError", [excErrMsg])
>
> The first argument of X509CertError is the certificate filename (in this
> case, Python docstring and actual usage do match).
Oops, good catch; will send patch.
thanks!!
iustin
