Code review comment for lp:~al-maisan/landscape-client/obsolete-results-v

[9]

+ message_context_store,

Please name this parameter "exchange_store", so it matches the name of the expected type (ExchangeStore) and it is shorter. Also, now that we have it, the ExchangeStore could be used to store also other types of information beside MessageContext's, so it makes sense to use a generic name for it.

+ self._store = message_context_store

Similarly please name this attribute "self._exchange_store", to make it clear which store is referring to (there is another attribute called "self._message_store", so the naming will be consistent).

[10]

+ else:
+ if "timestamp" not in message:
+ message["timestamp"] = int(self._reactor.time())
+ message_id = self._message_store.add(message)
+ if urgent:
+ self.schedule_exchange(urgent=True)
+ return message_id

I think you can save this else/indentation level and simply write:

+ if "timestamp" not in message:
+ message["timestamp"] = int(self._reactor.time())
+ message_id = self._message_store.add(message)
+ if urgent:
+ self.schedule_exchange(urgent=True)
+ return message_id

I personally like the pattern when you check a series of early-return conditions, before reaching the meat of a function/method, like

if this:
   return something
if that:
   return something_else

do_the_actual_thing

[11]

+ exchange_store = ExchangeStore(
+ os.path.join(config.data_path, "exchange.database"))

Just to avoid hard-coding this value you could add a property to l.broker.config.BrokerConfiguration, like:

@property
def exchange_store_path(self):
    return os.path.join(self.data_path, "exchange.database"))

and use it as

+ exchange_store = ExchangeStore(self.config.exchange_store_path)

[12]

+ exchange_store = ExchangeStore(
+ os.path.join(test_case.config.data_path, "exchange.database"))

Please add this object to the test_case attributes, so it can be used in tests:

+ test_case.exchange_store = ExchangeStore(
+ os.path.join(test_case.config.data_path, "exchange.database"))

after that you can avoid creating it explicitly in all the tests.

[13]

+ self.exchanger._store.get_message_context(message['operation-id']))

Once you have self.exchange_store as test attribute (see [12]) you should use it instead of accessing this private attribute.

Thanks!