Code review comment for lp:~lifeless/launchpad/lognamer

Hi Rob.

Thanks for extracting this logic. I have a few comments and I am confused
by a test change.

> === added file 'lib/canonical/launchpad/webapp/lognamer.py'
> --- lib/canonical/launchpad/webapp/lognamer.py 1970-01-01 00:00:00 +0000
> +++ lib/canonical/launchpad/webapp/lognamer.py 2010-06-27 06:41:28 +0000

webapp is deprecated. I think this would be better located at
lib/lp/services/logger/lognamer.py, maybe logger is too broad.

...

> +class LogNamer:
> + """Assign unique file names to logs being written from an app/script.
> +
> + LogNamer causes logs written from one process to be uniquely named. It is not
> + safe for use in multiple processes with the same output root - each process
> + must have a unique output root.
> + """

I see trailing whitespace. Wrap the doc at 78 characters.

...

> + def getFilename(self, log_serial, time):
> + """Get the filename for a given log serial and time."""
> + log_subtype = self._log_infix()
> + output_dir = self.output_dir(time) #XXX locks and scans, bad.

Avoid end comments. This looks like a comment rather than an XXX to denote
tech-debt.

...

> + def newId(self, now=None):
> + """Returns an (id, filename) pair for use by the caller.
> +
> + The ID is composed of a short string to identify the Launchpad instance
> + followed by an ID that is unique for the day.
> +
> + The filename is composed of the zero padded second in the day
> + followed by the ID. This ensures that reports are in date order when
> + sorted lexically.
> + """
> + if now is not None:
> + now = now.astimezone(UTC)
> + else:
> + now = datetime.datetime.now(UTC)
> + # We look up the error directory before allocating a new ID,
> + # because if the day has changed, errordir() will reset the ID
> + # counter to zero.
> + self.output_dir(now)
> + self._lock.acquire()
> + try:
> + self._last_serial += 1
> + newid = self._last_serial
> + finally:
> + self._lock.release()
> + subtype = self._log_infix()
> + day_number = (now - epoch).days + 1
> + log_id = '%s-%d%s%d' % (self._log_type, day_number, subtype, newid)
> + filename = self.getFilename(newid, now)
> + return log_id, filename

I am confused. The only changes I see in this method is prefix => infix,
yet I can see you changed the the last test for this method to verify
an error was raised.

Was the test wrong? Am I reading ``now.astimezone(UTC)`` wrong?

...

> === added file 'lib/canonical/launchpad/webapp/tests/test_lognamer.py'
> --- lib/canonical/launchpad/webapp/tests/test_lognamer.py 1970-01-01 00:00:00 +0000
> +++ lib/canonical/launchpad/webapp/tests/test_lognamer.py 2010-06-27 06:41:28 +0000
> ...
>
> + # Native timestamps are not permitted - UTC only.
> + now = datetime.datetime(2006, 04, 02, 00, 30, 00)
> + self.assertRaises(ValueError, namer.newId, now)