Code review comment for lp:~racb/maas/arch-detect

Several minor comments, but they're mostly about style; the approach
is good, afaict.

[1]

+    if macaddress:
+        return macaddress.node
+    else:
+        return None

Don't know how you feel about conditional expressions, but this might
be as clear but more concise as:

    return macaddress.node if macaddress else None

[2]

+    :param arch: Architecture name (in the pxelinux namespace, eg. 'arm' not
+                 'armhf').

Typically we align follow-on lines 4-spaces in from the first
line. Yay, another trivial nitpick :)

[3]

+                # Request was pxelinux.cfg/01-<mac>, so attempt fall back
+                # to pxelinux.cfg/default-<arch> for arch detection.

Is it default-<arch> or default.<arch>?

Update: the regex says either.

[4]

+            if pxelinux_arch == 'arm':
+                arch = 'armhf'
+            else:
+                arch = pxelinux_arch

One step in the direction of a lookup-table, and more concise:

            arch = {"arm": "armhf"}.get(pxelinux_arch, pxelinux_arch)

Er, but it's not very pretty. Never mind. Why is "arm" being sent
instead of "armhf"? Should we normalise on just "arm"?

[5]

+        response = self.client.get(reverse('pxeconfig'),
+                                   self.get_default_params())

We have followed the style of wrapping 4 spaces in most places:

        response = self.client.get(
            reverse('pxeconfig'), self.get_default_params())

Also, when we need to break onto a newline, we break at the opening
brace, so not like this:

        response = self.client.get(reverse('pxeconfig'),
            self.get_default_params())

Too much code on Launchpad ended up in the rightmost 20 columns
(there, like here, we chose to use a right margin) so we changed the
convention.

Hardly needs saying, but Emacs' python-mode does this all perfectly,
right out of the box ;)

[6]

+        ( # either a MAC

Make this a non-capturing group, i.e. (?:...)

+            {htype:02x}    # ARP HTYPE.
+            -
+            (?P<mac>{re_mac_address.pattern})    # Capture MAC.
+        | # or "default"
+            default
+              ( # perhaps with specified arch

Here too?

+                [.-](?P<arch>\w+) # arch
+                (-(?P<subarch>\w+))? # optional subarch

And here?

+              )?

[7]

+            params = {k: v
+                      for k, v in config_file_match.groupdict().iteritems()
+                      if v is not None}

We have tended to avoid single-letter variable names (sorry, again,
this is Launchpad engineering heritage showing its face, along with my
pedantry). Also, we decided to use the non-iter methods on dicts, I
think because they're spelled the same as in Python 3. I preferred to
use them, because they behave differently, and 2to3 translates them
anyway, but we, Red Squad, in days of old, agreed this. So, we'd write
this as:

            params = {
                key: value
                for key, value in config_file_match.groupdict().items()
                if value is not None
                }