Merge lp:~robru/gwibber/expiry into lp:~barry/gwibber/py3

On Sep 11, 2012, at 06:56 PM, Robert Bruce Park wrote:

>Robert Bruce Park has proposed merging lp:~robru/gwibber/expiry into lp:~barry/gwibber/py3.
>
>Requested reviews:
> Barry Warsaw (barry)
>
>For more details, see:
>https://code.launchpad.net/~robru/gwibber/expiry/+merge/123809
>
>Just a simple cache expiry method that scans the CACHE_DIR for any files
>older than 1 month and then deletes them.

This does need a test, and one is possible, but datetime will have to be
mocked so that you can fiddle with the dates to see if expire_old_avatars()
does the right thing.

=== modified file 'gwibber/gwibber/utils/avatar.py'
--- gwibber/gwibber/utils/avatar.py 2012-09-10 22:04:24 +0000
+++ gwibber/gwibber/utils/avatar.py 2012-09-11 18:55:42 +0000
> @@ -22,6 +22,10 @@
> import os
> import errno
> import logging
> +import datetime
> +import calendar
> +
> +from glob import glob
>
> from gi.repository import Gio, GLib, GdkPixbuf
> from hashlib import sha1
> @@ -67,3 +71,16 @@
> input_stream, 48, 48, True, None)
> pixbuf.savev(local_path, 'png', [], [])
> return local_path
> +
> + @staticmethod
> + def expire_old_avatars():
> + # TODO actually call this from somewhere.
> + limit = datetime.date.today() - datetime.timedelta(weeks=4)

It's possible we'll want the cache lifetime to be configurable at some point,
but I guess we can worry about that later.

Also, IME expiry arithmetic is easier to follow if it's done slightly
differently. With the above code, and the following condition, you have to
sort of stop and think about the meaning. If instead you *added* 4 weeks to
the file's mtime, and then compared that to see if it was greater than today,
it's typically easier to follow the logic. I've done it both ways over the
years, and the latter just seems to be more readable.

> + limit = calendar.timegm(limit.timetuple())
> +
> + for cached_file in glob(os.path.join(CACHE_DIR, '*')):

Rather than using glob() on '*', os.listdir() is probably faster and more
idiomatic.

> + try:
> + if os.stat(cached_file).st_mtime < limit:
> + os.unlink(cached_file)
> + except OSError:
> + pass

What OSErrors do you expect to suppress? Until Python 3.3 comes out, it's
always a good idea to reraise errnos you're not expecting. E.g. in this case,
you can ignore ENOENTs, but anything else would be unexpected.

Thanks for the branch. I'll write a test and merge.

On Sep 11, 2012, at 08:10 PM, Barry Warsaw wrote:

>Also, IME expiry arithmetic is easier to follow if it's done slightly
>differently. With the above code, and the following condition, you have to
>sort of stop and think about the meaning. If instead you *added* 4 weeks to
>the file's mtime, and then compared that to see if it was greater than today,
>it's typically easier to follow the logic. I've done it both ways over the
>years, and the latter just seems to be more readable.

Yeah, never mind. What you wrote is fine.

On 12-09-11 03:10 PM, Barry Warsaw wrote:
> This does need a test, and one is possible, but datetime will have to be
> mocked so that you can fiddle with the dates to see if expire_old_avatars()
> does the right thing.

Ah, hadn't considered mocking datetime. I was thinking it might be
possible to mock out the cache dir and then didle the mtime to be more
than 4 weeks old, but I wasn't sure.

>> + @staticmethod
>> + def expire_old_avatars():
>> + # TODO actually call this from somewhere.
>> + limit = datetime.date.today() - datetime.timedelta(weeks=4)
>
> It's possible we'll want the cache lifetime to be configurable at some point,
> but I guess we can worry about that later.

I disagree, because I still believe that most of the protocols are going
to provide us with new URLs when the avatar image changes, so this is
highly unlikely to be something that any user is ever going to care
about. It's just a simple matter of housekeeping. Probably even a longer
time period would be fine.

> Also, IME expiry arithmetic is easier to follow if it's done slightly
> differently. With the above code, and the following condition, you have to
> sort of stop and think about the meaning. If instead you *added* 4 weeks to
> the file's mtime, and then compared that to see if it was greater than today,
> it's typically easier to follow the logic. I've done it both ways over the
> years, and the latter just seems to be more readable.

Oh Barry, you're so funny.

>> + limit = calendar.timegm(limit.timetuple())
>> +
>> + for cached_file in glob(os.path.join(CACHE_DIR, '*')):
>
> Rather than using glob() on '*', os.listdir() is probably faster and more
> idiomatic.

Ah, I wasn't aware of os.listdir. Yay learning!

>> + try:
>> + if os.stat(cached_file).st_mtime < limit:
>> + os.unlink(cached_file)
>> + except OSError:
>> + pass
>
> What OSErrors do you expect to suppress?

The documentation indicated that calling os.unlink on a directory would
raise OSError. I don't *expect* any directories in the cache dir, but I
just thought it'd be better to fail gracefully just in case some
unexpected breakage occurs.

> Until Python 3.3 comes out, it's
> always a good idea to reraise errnos you're not expecting. E.g. in this case,
> you can ignore ENOENTs, but anything else would be unexpected.
>
> Thanks for the branch. I'll write a test and merge.

Thanks again, Barry!

>
>

On Sep 11, 2012, at 09:07 PM, Robert Bruce Park wrote:

>Oh Barry, you're so funny.

"I'm funny how? I mean, funny like a clown? I amuse you? I make you laugh?"

17 years of hacking Python will do that to you.

:)