Code review comment for lp://staging/~tealeg/landscape-client/scoped_session_ids

Nice work in keeping these branches small, it really helps. +1!

#1

         # When re-synchronisation occurs we don't want any previous messages
         # being sent to the server, dropping the existing session_ids means
         # that messages sent with those IDs will be dropped by the broker.
- self._message_store.drop_session_ids()
+ self._message_store.drop_session_ids(scopes)

Please expand the comment mentioning that only the session IDs for the specified scopes will be dropped (or all of them if scopes is None).

#2

- def get_session_id(self):
+ def get_session_id(self, scope=None):

There should be a @param entry for the new parameter, in the method docstring.

#3

- def drop_session_ids(self, scope=None):
+ def drop_session_ids(self, scopes=None):
         """
         Drop all session ids.
         """

Please update the method docstring and document the new parameter.

#4

+ test_session_id = self.successResultOf(
+ self.client.broker.get_session_id(scope="test"))

Opinionated nitpick: since this is using 2 lines anyways, I find it generally better to split it like this:

        deferred = self.client.broker.get_session_id(scope="test")
        test_session_id = self.successResultOf(deferred)

because imo there's less parsing work going on in one's mind when reading the code. This is really minor (and I guess a counter argument could be made), so your call.

#5

+ global_scope = []
+ self.reactor.fire("resynchronize-clients", global_scope)

This is probably a leftover, you can just:

        self.reactor.fire("resynchronize-clients")

which defaults to the global scope.

#6

+ self.assertMessages(messages, [{"type": "empty"}])

This doesn't test which one of the 2 sessions IDs generated the message exactly, it'd be nice to use 2 different message types I guess. Maybe it's not needed from a strict unit-test point of view, since I believe test_drop_multiple_scopes from test_store.py covers that. Your call.