[tor-dev] Code Review: or-applet

[Damian Johnson]

Hi Yawning, I just took a quick peek at your or-applet
(https://github.com/Yawning/or-applet). Nice work! Added it to Stem's
examples page.

Just a few thoughts. Nothing very substantial...

> def _format_status(status):
>     if status == CircStatus.LAUNCHED:
>         return 'LAUNCHED (circuit ID assigned to new circuit)'
>     elif status == CircStatus.BUILT:
>         return 'BUILT (all hops finished, can now accept streams)'
>     elif status == CircStatus.EXTENDED:
>         return 'EXTENDED (one more hop has been completed)'
>     elif status == CircStatus.FAILED:
>         return 'FAILED (circuit closed (was not built))'
>     elif status == CircStatus.CLOSED:
>         return 'CLOSED (circuit closed (was built))'
>     return str(status)

Very minor thing but I'd probably opt for the following...

  FORMAT_STATUSES = {
    CircStatus.LAUNCHED: 'LAUNCHED (circuit ID assigned to new circuit)',
    CircStatus.BUILT: 'BUILT (all hops finished, can now accept streams)',
    CircStatus.EXTENDED: 'EXTENDED (one more hop has been completed)',
    CircStatus.FAILED: 'FAILED (circuit closed (was not built))',
    CircStatus.CLOSED: 'CLOSED (circuit closed (was built))',
  }

  def _format_status(status):
    return FORMAT_STATUSES.get(status, str(status))

Ditto for the rest of the status_icon.py functions.

> def _build_dynamic_menu(self):
>     circuits = self._ctl.get_circuits()
>     if circuits == None:
>         item = Gtk.MenuItem('No circuits established')
>         item.set_sensitive(False)
>         self._menu.append(item)
>         return
>     streams = self._ctl.get_streams()

The get_circuits() method will never return None as you're using it
here. It'll either return a list or raise a stem.ControllerError. I
think what you actually want is...

  circuits = self._ctl.get_circuits(None)

That way it'll return None rather than raise an exception if it fails.

> our_streams.append('[' + stream.id + ']: ' + stream.target)

In python string concatenation is discouraged unless it gives you
readability benefits. It's slightly more performant to use format() or
the following...

  our_streams.append('[%s]: %s' % (stream.id, stream.target))

That said, really doesn't matter. :)

> if len(our_streams) == 0:
>   ...

The boolean value of an empty list is False, so this is usually done as...

  if our_streams:
    ...

> if self._control == None:

Minor thing but None comparison is the one time it's suggested that
you use the 'is' keyword (ie, 'if self._control is None:').

> class PopupMenu:

It's generally a good idea to always extend object...

  class PopupMenu(object):

> def is_newnym_available(self):
>     if self._control == None:
>         return False
>     return self._control.is_newnym_available()

Hmmm, a goal I had for stem Controller objects was that they could be
gracefully disconnected and reconnected. The Controller's
is_newnym_available() already checks is_alive() and returns False if
you're disconnected from tor.

Ideally self._control should never be None. However, if you've never
had an *initial* tor connection then I'm not sure that it's possible
to instantiate a Controller. If so then that's a stem issue I'll need
to think about...

Cheers! -Damian