Repository: juju-solutions/charms.reactive

Issue:
toggle_state default for active is error-prone
`toggle_state` takes an optional `active` param that lets you specify whether the state should be on or off, instead of it being switched from its previous value.  Having a default value of `None` is problematic because `None` can easily be mistaken for a "truthy" `False` when dealing with relation data, which leads to the state being unexpectedly set.  For example, the following breaks:

```python
conv.toggle_state('{relation_name}.feature_x',
                  active=conv.get_remote('feature_x_available'))
```

This can also be more subtle because `None and <anything>` returns `None` instead of `False` as one might expect.  For example, this also breaks:

```python
name = conv.get_remote('name')
version = conv.get_remote('version')
conv.toggle_state('{relation_name}.ready', name and version)
```


Discussion:
kwmonroe: There was some chatter on #juju about not using a string representation for toggle (instead using `object()`).  I'm fine with this as-in, or with that changed.
stub42: This needs a test, which I suspect is trivial.

Patch:
diff --git a/charms/reactive/relations.py b/charms/reactive/relations.py
index 173ef28..a8c1499 100644
--- a/charms/reactive/relations.py
+++ b/charms/reactive/relations.py
@@ -30,7 +30,9 @@ from charms.reactive.bus import _load_module
 from charms.reactive.bus import StateList
 
 
-ALL = '__ALL_SERVICES__'
+# arbitrary obj instances to use as defaults instead of None
+ALL = object()
+TOGGLE = object()
 
 
 class scopes(object):
@@ -296,13 +298,13 @@ class RelationBase(with_metaclass(AutoAccessors, object)):
         """
         return self.conversation(scope).is_state(state)
 
-    def toggle_state(self, state, active=None, scope=None):
+    def toggle_state(self, state, active=TOGGLE, scope=None):
         """
         Toggle the state for the :class:`Conversation` with the given scope.
 
         In Python, this is equivalent to::
 
-            relation.conversation(scope).toggle_state(state)
+            relation.conversation(scope).toggle_state(state, active)
 
         See :meth:`conversation` and :meth:`Conversation.toggle_state`.
         """
@@ -549,7 +551,7 @@ class Conversation(object):
             return False
         return self.key in value['conversations']
 
-    def toggle_state(self, state, active=None):
+    def toggle_state(self, state, active=TOGGLE):
         """
         Toggle the given state for this conversation.
 
@@ -565,7 +567,7 @@ class Conversation(object):
 
         This will set the state if ``value`` is equal to ``foo``.
         """
-        if active is None:
+        if active is TOGGLE:
             active = not self.is_state(state)
         if active:
             self.set_state(state)


Test patch:
diff --git a/tests/test_relations.py b/tests/test_relations.py
index c4977da..d49c3e1 100644
--- a/tests/test_relations.py
+++ b/tests/test_relations.py
@@ -171,6 +171,11 @@ class TestRelationBase(unittest.TestCase):
         rb.conversation.assert_called_once_with('scope')
         conv.toggle_state.assert_called_once_with('state', 'active')
 
+        conv.toggle_state.reset_mock()
+        rb.toggle_state('state')
+        conv.toggle_state.assert_called_once_with('state',
+                                                  relations.TOGGLE)
+
     def test_set_remote(self):
         conv = mock.Mock(name='conv')
         rb = relations.RelationBase('relname', 'unit')
@@ -391,12 +396,14 @@ class TestConversation(unittest.TestCase):
 
         conv.toggle_state('foo')
         self.assertEqual(conv.remove_state.call_count, 1)
+        conv.toggle_state('foo', None)
+        self.assertEqual(conv.remove_state.call_count, 2)
         conv.toggle_state('foo')
         self.assertEqual(conv.set_state.call_count, 1)
         conv.toggle_state('foo', True)
         self.assertEqual(conv.set_state.call_count, 2)
         conv.toggle_state('foo', False)
-        self.assertEqual(conv.remove_state.call_count, 2)
+        self.assertEqual(conv.remove_state.call_count, 3)
 
     @mock.patch.object(relations.hookenv, 'relation_set')
     @mock.patch.object(relations.Conversation, 'relation_ids', ['rel:1', 'rel:2'])