Merge lp:~doanac/utah/pluggable-battery into lp:utah

[Andy Doan]

kind of went on a detour today. the "puts" system is going to be deployed in the lab pretty soon and will cause us to need to support multiple battery implementations in the UTAH client.

I'm not a huge fan of this approach, but we aren't using pkg_resources anywhere else in our code, so thought this might suffice.

[Max Brustkern (nuclearbob)]

This looks reasonable to me, but I don't have a good way to test it.

[Andy Doan (doanac)]

I added a temp class class to the file like:

class _dummyBattery(object):
    @staticmethod
    def get_name():
        return 'dummy'

    def get_data(self):
        return {'foo': 'bar'}

[Javier Collado (javier.collado)]

The code looks fine, but let me make a few comments:

- _batteries
I think the client script shouldn't take care of caching this value. What about
exposing this data in the battery module directly? The get_implementations
function could be removed as well:

utah/client/battery.py
implementations = {}
for name, cls in globals().iteritems():
    if name.endswith("Battery"):
        implementations[cls.get_name()] = cls

utah/client.py
battery.implementations

- %(default)s in battery argument help string
%(default)s could be used in `batt_help` to let the user know from the command
line what is the default battery implementation.

- keys() iteration
Given that in a dictionary the iteration happens on its keys by default, I
prefer to avoid using the keys() method for that.

- utah.client.battery._SWBattery.get_name static method
Is this something that will need to be calculated on the fly for other
implementations? If not, then a class attribute is just fine.

- utah.client.battery.get
Using this function to get the battery implementation every time, makes the interface
more verbose:
battery.get().get_data()

What about having another class, something like `EmptyBattery` that implements
`get_data` in a way that raises the exception for the error not to having
selected any implementation?

class EmptyBattery:
    def get_data(self):
        raise UTAHException('No battery implementation selected')

battery = EmptyBattery()

This way, the interface can still be the same as in the previous version:
from utah.client.battery import battery
battery.get_data()

and the code that checks for the implementation is not executed repeatedly.

- RuntimeError
To be consistent with the rest of the code, this should be a UTAHException.

[Andy Doan (doanac)]

nice suggestions. I think I basically took your idea, but not quite exactly. I kept the "get_name" static method, because I wasn't sure if it would need to be computed or not in the future.

review: Needs Resubmitting

[Javier Collado (javier.collado)]

The diff is now empty. It looks like lp:~doanac/utah/pluggable-battery was
overwritten with dev.

[Andy Doan (doanac)]

oops - should be updated now

review: Needs Resubmitting

[Javier Collado (javier.collado)]

That's strange, the diff is still empty.

I've taken a look at the branch directly and the changes look better now. In
particular, I like to still have the easy interface like: battery.get_data()

Anyway, still there are a couple of things I don't like much:

- set_implementation

A set method looks and kind of implies that the user of the code know how it
works under the hood.

What do you think about having a property called 'implementation' whose getter
method returns the name of the current implementation and the setter sets the
right implementation based only on the name.

This way, the client code just needs something like:
battery.implementation = <implementation_name>

and also, the dictionary of class don't really need to exposed, just the
implementation names so that the name passed through the command line can be
validated.

- setup_battery

This function raises ArgumentTypeError if the passed implementation name isn't
available. This should be done by `argparse` directly using a `choices` list
(i.e. a list of implementation names) in the call to `add_argument`.

[lp:~doanac/utah/pluggable-battery]

864. By Andy Doan on 2013-04-18

review comments from javier

some good ideas by javier

[Andy Doan (doanac)]

thanks for the great feedback. revno 864 cleans up stuff from your last comment.

review: Needs Resubmitting

[lp:~doanac/utah/pluggable-battery]

865. By Andy Doan on 2013-04-18

remove unused import

[Javier Collado (javier.collado)]

Thanks for the effort on the branch. I think the changes are pretty good shape
now.

Running static analysis tools I found a few issues:

- flake8 output
./utah/client/tests/test_battery.py:21:1: F401 'UTAHClientError' imported but unused
./utah/client/battery.py:25:1: E302 expected 2 blank lines, found 1

- pep257 output
battery.py:27:4: PEP257 Class docstring should have 1 blank line around them.
battery.py:41:4: PEP257 Exported definitions should have docstrings.
battery.py:125:4: PEP257 Exported definitions should have docstrings.
battery.py:128:4: PEP257 Exported definitions should have docstrings.
test_battery.py:30:8: PEP257 First line should be in imperative mood ('Do', not 'Does').
test_battery.py:38:8: PEP257 Return value type should be mentioned.

Finally, regarding the property code, it's fine the way it is now, but I wanted
to mention that I like the decorator syntax in the documentation just in case
you want to consider it:

class C(object):
    def __init__(self):
        self._x = None

    @property
    def x(self):
        """I'm the 'x' property."""
        return self._x

    @x.setter
    def x(self, value):
        self._x = value

[lp:~doanac/utah/pluggable-battery]

866. By Andy Doan on 2013-04-19

pep257/pyflake warnings

[Andy Doan (doanac)]

should be cleaned up now.

review: Approve

[Javier Collado (javier.collado)]

Looks good now. Thanks.

review: Approve

=== modified file 'client.py'
--- client.py	2013-04-04 13:13:34 +0000
+++ client.py	2013-04-19 15:36:27 +0000
@@ -24,6 +24,7 @@
 import os
 
 from utah.url import url_argument
+from utah.client.battery import battery
 from utah.client.runner import Runner
 from utah.client.state_agent import StateAgentYAML
 from utah.client.result import Result, ResultYAML, ResultJSON
@@ -73,6 +74,10 @@
                         help='append to output')
     parser.add_argument('-d', '--debug', action='store_true',
                         help='Print debugging output')
+    parser.add_argument('-b', '--battery',
+                        default=battery.implementation,
+                        choices=battery.implementations(),
+                        help='Choose which battery implementation to use.')
 
     return parser
 
@@ -87,6 +92,10 @@
                         format='%(asctime)s - %(levelname)s - %(message)s')
 
 
+def setup_battery(battery_name):
+    battery.implementation = battery_name
+
+
 def main():
     """Interpret command line arguments and run tests."""
     # TODO: write <2.7 optparse version and set based on version of python
@@ -102,6 +111,7 @@
         sys.exit(ReturnCodes.INVALID_USER)
 
     setup_logging(args.debug)
+    setup_battery(args.battery)
 
     logging.debug(args)
     logging.info(platform.uname())
 
=== modified file 'utah/client/battery.py'
--- utah/client/battery.py	2013-04-04 13:03:51 +0000
+++ utah/client/battery.py	2013-04-19 15:36:27 +0000
@@ -20,10 +20,17 @@
 import re
 import logging
 
-
-class _Battery(object):
-
-    """Battery information gathering."""
+from utah.client.exceptions import UTAHClientError
+
+
+class _SWBatteryImpl(object):
+
+    """Gathers battery information via the kernel's "power_supply" class.
+
+    .. seealso::
+       https://www.kernel.org/doc/Documentation/power/power_supply_class.txt
+
+    """
 
     POWER_SUPPLY_DIR = '/sys/class/power_supply'
     BATTERY_DIR_REGEX = re.compile(r'bat.*', re.IGNORECASE)
@@ -32,6 +39,11 @@
     def __init__(self):
         self.filenames = None
 
+    @staticmethod
+    def get_name():
+        """return the implementation name displayed to the user."""
+        return 'software'
+
     def get_data(self):
         """Get data from the battery information files.
 
@@ -103,5 +115,34 @@
                       .format(', '.join(filenames)))
         return filenames
 
-# Use singleton object to avoid multiple unneeded initializations
+
+class _Battery(object):
+    def __init__(self):
+        self._impl = _SWBatteryImpl()
+        self._impls = {}
+
+        for name, cls in globals().iteritems():
+            if name.endswith("BatteryImpl"):
+                self._impls[cls.get_name()] = cls
+
+    def implementations(self):
+        """return the battery implementation names."""
+        return self._impls.keys()
+
+    def get_data(self):
+        """return the battery data from the implementation."""
+        if not self._impl:
+            raise UTAHClientError('No battery implementation set')
+        return self._impl.get_data()
+
+    def _get_impl(self):
+        if not self._impl:
+            return None
+        return self._impl.get_name()
+
+    def _set_impl(self, name):
+        self._impl = self._impls[name]()
+
+    implementation = property(_get_impl, _set_impl)
+
 battery = _Battery()
 
=== added file 'utah/client/tests/test_battery.py'
--- utah/client/tests/test_battery.py	1970-01-01 00:00:00 +0000
+++ utah/client/tests/test_battery.py	2013-04-19 15:36:27 +0000
@@ -0,0 +1,54 @@
+# Ubuntu Testing Automation Harness
+# Copyright 2012 Canonical Ltd.
+
+# This program is free software: you can redistribute it and/or modify it
+# under the terms of the GNU General Public License version 3, as published
+# by the Free Software Foundation.
+
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License along
+# with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Test our battery interface."""
+
+
+import unittest
+
+from utah.client.battery import battery
+
+
+class TestBattery(unittest.TestCase):
+
+    """Basic testing of the battery interface."""
+
+    def test_listing(self):
+        """Ensure we properly discover the implementations."""
+        self.assertTrue('software' in battery.implementations())
+
+    def test_default(self):
+        """Make sure battery initialization is initialzed correctly."""
+        self.assertIsNotNone(battery.get_data())
+
+    def test_non_default(self):
+        """Perform check to see if non-default battery works.
+
+        :returns: nothing - the inner function is confusing pep257
+
+        """
+        class dummy(object):
+            @staticmethod
+            def get_name():
+                return 'dummy'
+
+            def get_data(self):
+                return {'foo': 'bar'}
+
+        battery._impls[dummy.get_name()] = dummy
+        self.assertTrue('dummy' in battery.implementations())
+
+        battery.implementation = dummy.get_name()
+        self.assertEquals(battery.get_data()['foo'], 'bar')