Code review comment for lp://staging/~fwereade/pyjuju/restrict-relation-broken-context

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

Nice work, some minors.

[0]

Cache instance attributes could always use a documentation line on
what's being cached.

hook.py line 269

self._relation_cache = None

[1]

set_value/set

        return fail(NotImplementedError(
            "Cannot change settings in broken relation"))

That doesn't seem like the appropriate error in this
context. Something more specific to the problem at hand seems
appropriate, ie. RelationBrokenContextError

[2] pep8 nags, trailing newline at EOF, over 80 (unrelated to diff) in hook.py

unit_state = yield service_state_manager.get_unit_state(self._unit_name)

[3]

seems like the mixin here should be merged into hookcontexttestbase,
all the tests have the same pair of base classes.

HookContextTest(HookContextTestBase, CommonHookContextTestsMixin):

review: Approve

« Back to merge proposal