Code review comment for lp:~mandel/ubuntuone-client/improve-fs-notifications-logging

Revision history for this message
Natalia Bidart (nataliabidart) wrote :

* The following code is not needed since the memento handler accepts multiple strings in one single call:

119 + def _assert_logs(self, events):
120 + """Assert the debug logs."""
121 + logs = self._get_fs_logs(events)
122 + for msg in logs:
123 + self.assertTrue(self.memento.check_debug(msg))

you just have to do:

        self.assertTrue(self.memento.check_debug(*self._get_fs_logs(events)))

* I also think that all the new helper functions could be improve using list comprehensions, for example:

86 + def _get_event_msgs(self, events):
87 + """Return the pushing event logs."""
88 + logs = []
89 + for e in events:
90 + logs.append('Pushing event %s to processor.' % e)
91 + return logs

can be better read as:

86 + def _get_event_msgs(self, events):
87 + """Return the pushing event logs."""
88 + msg = 'Pushing event %s to processor.'
89 + return [msg % e for e in events]

Anyways, if we use the former to create generators instead of lists in memory, we can improve all the helpers having only one like this:

def _assert_logs(self, events):
    logs = []

    msg = 'Pushing event %r to processor.'
    logs.extend(msg % e for e in events)

    msg = 'Got from ReadDirectoryChangesW %r.'
    logs.extend(msg % [(WINDOWS_ACTIONS_NAMES[a], p) for a, p in actions]
            for actions in self.raw_events)

    msg = 'Is path %r a dir? %r'
    logs.extend(is_dir % data for data in self.paths_checked)

    self.assertTrue(self.memento.check_debug(*logs))

* Also, always user %r to log variables, not %s, since the latter can fail for non-ascii strings.

* For every addHanlder, you need to queue a removeHandler cleanup function.

* The rest looks good!

review: Needs Fixing

« Back to merge proposal