Merge lp:~javier.collado/utah/battery-measurements-logging into lp:utah

[Javier Collado]

As explained in the commit message:

-----
The battery module creates a singleton object at import time to take care of
gathering the data related to all battery measurements. Unfortunately, if a
logging method is used, logging.basicConfig is called with default arguments.
Later, UTAHs configure logging function believes logging is already configured
and as a results logging ends up being wrongly configured.

To fix this, this change delays the computation done at import time and does
them the first time the object is used which is later than logging has been
configured.
----

Note that this merge request is needed by the changes in:
https://code.launchpad.net/~javier.collado/utah/reboot-support/+merge/151256

for them to work properly with regard to logging.

[Andy Doan (doanac)]

Here's my concern (and I'm not sure if its worth considering):

If someone ever did something innocent like add a "logging.info" to _Battery.__init__, then this bug will come back. It seems like the real bug is at the end of the file:

  battery = _Battery()

I wonder if we should change that to be something like:

_inst = None
def get_battery():
   if not inst:
      _inst = _Battery()
    return _inst

then you'd have to refactor who uses this, so maybe its overkill

[Javier Collado (javier.collado)]

@Andy

Yes, I though about having a class method to cache the instance instead of
caching the file names. However, from the client code perspective, I though
this looked cleaner:
battery.get_data()

than this:
_Battery.get_battery().get_data()

The risk of potentially writing a logging message before it's been configured
it's all over the code, so probably the best way to address is to do something
like detect when current logging configuration isn't the intended one and fail
to force the developer to fix problems like the one in this merge proposal.

Anyway, this is out of the scope of the merge proposal, but we can open a bug
if you wish and work on a bettter solution.

[Andy Doan (doanac)]

On 03/14/2013 07:21 AM, Javier Collado wrote:
> @Andy
> Anyway, this is out of the scope of the merge proposal, but we can open a bug
> if you wish and work on a bettter solution.

no bug needed. just wanted to get your thoughts.
+1

=== modified file 'utah/client/battery.py'
--- utah/client/battery.py	2013-02-08 14:19:29 +0000
+++ utah/client/battery.py	2013-03-13 12:19:21 +0000
@@ -11,7 +11,7 @@
     BATTERY_FILE_REGEX = re.compile(r'(.*_now|capacity|status)')
 
     def __init__(self):
-        self.filenames = self._get_files()
+        self.filenames = None
 
     def get_data(self):
         """Get data from the battery information files
@@ -38,6 +38,10 @@
                 except ValueError:
                     return data_str
 
+        # Cache file names for later use
+        if self.filenames is None:
+            self.filenames = self._get_files()
+
         return {os.path.basename(filename): read_file(filename)
                 for filename in self.filenames}