Code review comment for lp:~xnox/apt-btrfs-snapshot/py3

On Jun 11, 2012, at 01:20 PM, Dmitrijs Ledkovs wrote:

>Dmitrijs Ledkovs has proposed merging lp:~dmitrij.ledkov/apt-btrfs-snapshot/py3 into lp:apt-btrfs-snapshot.
>
>Requested reviews:
> Canonical Foundations Team (canonical-foundations)
> Michael Vogt (mvo)
>
>For more details, see:
>https://code.launchpad.net/~dmitrij.ledkov/apt-btrfs-snapshot/py3/+merge/109632
>
>python3 port and other fixes

=== modified file 'apt-btrfs-snapshot'
--- apt-btrfs-snapshot 2011-07-19 11:42:45 +0000
+++ apt-btrfs-snapshot 2012-06-11 13:19:19 +0000
> @@ -1,4 +1,4 @@
> -#!/usr/bin/python
> +#!/usr/bin/python3
> # Copyright (C) 2011 Canonical
> #
> # Author:
> @@ -17,6 +17,8 @@
> # this program; if not, write to the Free Software Foundation, Inc.,
> # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>
> +from __future__ import print_function

This import got me thinking. If apt-btrfs-snapshot (the script) will only run
in Python 3, then clearly we don't need this. If it could run in Python 2
(e.g. explicitly invoked ignoring the #! line, or if you want to hold out the
possibility of reverting to Python 2), then the import is fine, but possibly
incomplete.

(Aside: print_function was introduced in Python 2.6.0a2:
http://docs.python.org/py3k/library/__future__.html)

It might be incompletely because you may also need to import unicode_literals
(FWIW, just as a blanket rule, I always import those two plus
absolute_import). The other thing you'll need to check is that the _()
function is defined properly for cross-py2/py3 compatibility. It's not clear
from the diff if this is the case.

Here's the thing: you always want _() to return unicode strings, and you want
the input to be unicode strings. In Python 2, gettext.gettext() is *not* the
right thing to use, you need gettext.ugettext(). In Python 3,
gettext.gettext() does operate on and return unicodes.

You can do something like this to make it work right (this is pulled from the
c-j code in u-m).

def setup_gettext():
    domain = 'foo'
    localedir = os.environ.get('LOCPATH')
    t = gettext.translation(domain, localedir=localedir, fallback=True)
    try:
        return t.ugettext
    except AttributeError:
        return t.gettext

Then...

_ = setup_gettext()

OTOH, if the script will always run under Python 3, most of that won't be
necessary.

=== modified file 'apt_btrfs_snapshot.py'
--- apt_btrfs_snapshot.py 2011-10-17 08:05:22 +0000
+++ apt_btrfs_snapshot.py 2012-06-11 13:19:19 +0000
> @@ -16,9 +16,10 @@
> # this program; if not, write to the Free Software Foundation, Inc.,
> # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>
> +from __future__ import print_function

Even though you're not using gettext, I'd still consider including
unicode_literals here. The problem is that if in Python 2, you're operating
on 8-bit strings, they'll magically become unicodes in Python 3, and then
you're just asking for trouble. If the literals really should be 8-bit
strings (a.k.a. bytes) in both versions, then import unicode_literals and
prefix your byte literals with b''. That way, the intentions are clear, and
things will work the same in both versions.

> @@ -57,14 +58,16 @@
> """ a list of FstabEntry items """
> def __init__(self, fstab="/etc/fstab"):
> super(Fstab, self).__init__()
> - for line in map(string.strip, open(fstab)):
> - if line == "" or line.startswith("#"):
> - continue
> - try:
> - entry = FstabEntry.from_line(line)
> - except ValueError:
> - continue
> - self.append(entry)
> +
> + with open(fstab) as fstab_file:
> + for line in [l.strip() for l in fstab_file]:

Not that you have to change it, but you can also use a generator expression
here if you want to save on the cost of creating and destroying a concrete
list. E.g.

            for line in (l.strip() for l in fstable_file):