Code review comment for lp:~fwereade/pyjuju/cobbler-zk-connect-error-messages

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

Looks generally good, with a few exceptions:

[1]

+ self.name = type(error).__name__ if error else "error"
+ self.action = action or "interacting with provider"
+ self.message = message or str(error)

     def __str__(self):
     - return "ProviderError: Interaction with machine provider failed: %r" \
     - % self.error
     + return "Unexpected %s %s: %s" % (self.name, self.action, self.message)

This sounds somewhat confusing, and quite likely to turn out
in bad error messages. There's no way to make sense, or to
introduce new messages, without looking at the implementation.
Improving on the original error message is good, but please
use something more straightforward here and in the call sites.

[2]

- raise FormulaError(path, "Must be a ZIP-file (%s)." % str(exc))
+ raise FormulaError(path, "must be a zip file (%s)" % exc)

Our error messages start with a capitalized word.

[3]

             raise FormulaError(path,
      - "Archive does not contain required file: "
      - "\"metadata.yaml\".")
      + "archive does not contain required file "
      + "\"metadata.yaml\"")

Same thing.

[4]

- @param master: if True, machine will initialize the ensemble admin
+ `master` if True, machine will initialize the ensemble admin
             and run a provisioning agent.

I suspect this is the first time I see this notation, and our overall comment
style is starting to feel like a soup. We have at least three different kinds
of comments, besides the broken ones. Please revert the changes to parameter
names and raising/returning specifically so that this branch can be merged
and let's talk about styling in a different thread.

review: Approve

« Back to merge proposal