Code review comment for lp:~jimbaker/pyjuju/hook-command-error-cases

Revision history for this message
Clint Byrum (clint-fewbar) wrote :

Hi Jim. This is a pretty big merge, so perhaps next time use lbox propose -cr so we can review w/ rietveld?

Anyway, I think it all LGTM .. one bit of feedback:

99 - raise RelationStateNotFound()
100 + parts = relation_ident.split(":")
101 + if len(parts) != 2 or not parts[1].isdigit():
102 + raise InvalidRelationIdentity(relation_ident)
103 + else:
104 + raise RelationStateNotFound()

I don't really understand checking the format at this point.. we already know its not found, and thats what a user trying to debug their charm would want to know. The format problem would likely be *more* confusing. So I think this check, and the extra test for it, can just be dropped.

review: Needs Fixing

« Back to merge proposal