Merge lp://qastaging/~james-page/charm-helpers/caching_hookenv into lp://qastaging/charm-helpers
Proposed by
James Page
| Status: | Merged |
|---|---|
| Merged at revision: | 31 |
| Proposed branch: | lp://qastaging/~james-page/charm-helpers/caching_hookenv |
| Merge into: | lp://qastaging/charm-helpers |
| Diff against target: |
448 lines (+166/-58) 2 files modified
charmhelpers/core/hookenv.py (+66/-13) tests/core/test_hookenv.py (+100/-45) |
| To merge this branch: | bzr merge lp://qastaging/~james-page/charm-helpers/caching_hookenv |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Matthew Wedgwood (community) | Approve | ||
|
Review via email:
|
|||
Description of the change
Various refactoring + additions to hookenv
1) cached decorator
Allows caching of function + args to avoid repeated calls out the cli
to juju commands.
2) Normalizing missing unit/config/
relation_
config('missing') == None
unit_get('missing') == None
3) Normalized use of the Serializable object a bit
And added a unit test to cover missing attribute lookups
Question: maybe this should actually return 'None' rather than throwing
an exception inline with 2).
4) Tweaks some tests to be closer to juju behaviour with json
5) Tidied misc pep8/pylint warnings.
To post a comment you must log in.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
Good stuff.
I have very slight reservations about the way that flush works because it's very imprecise about the keys it removes. It doesn't matter now, but it could be confusing if someone were to extend the module in the future. It's also arguable whether it would really matter if the cache was cleared unexpectedly.
+1