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'? > + , ("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.) > + , ("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. > + , ("X509CertError", [excErrMsg]) The first argument of X509CertError is the certificate filename (in this case, Python docstring and actual usage do match). -- Dato Simó | [email protected] Corp Fleet Management / Ganeti SRE (Dublin)
