Merge lp:~james-w/pkgme/dont-make-existing-dir into lp:pkgme

[James Westby]

Allow overriding the location of the log file.

Hi,

A small change to allow configuring the location of the log file. We're not
going to want it hidden away when pkgme is used by pkgme-service, so having
a way to customize it will be great.

Thanks,

James

[lp:~james-w/pkgme/dont-make-existing-dir]

93. By James Westby on 2012-01-18

Merge trunk.

94. By James Westby on 2012-01-18

Actually treat PKGME_LOG_DIR as a dir.

[Jonathan Lange (jml)]

Two things:

 * The test has a copy/pasted comment that's not correct. "If PKGME_LOG_DIR is set, we log there" would be appropriate.

 * I would have written the thing like this:

    log_dir = os.environ.get("PKGME_LOG_DIR", None)
    log_filename = 'pkgme.log'
    if log_dir is None:
        log_dir = os.path.expanduser(os.path.join('~', '.cache', 'pkgme'))
    return os.path.join(log_dir, log_filename)

Which isn't superior (two calls to o.p.j), but I thought it might at least be interesting to share.

review: Needs Fixing

[lp:~james-w/pkgme/dont-make-existing-dir]

95. By James Westby on 2012-01-19

Fixes from review, thanks jml.

[James Westby (james-w)]

On Thu, 19 Jan 2012 11:58:35 -0000, Jonathan Lange <email address hidden> wrote:
> Review: Needs Fixing
>
> Two things:
>
> * The test has a copy/pasted comment that's not correct. "If PKGME_LOG_DIR is set, we log there" would be appropriate.

Fixed, thanks.

> * I would have written the thing like this:
>
> log_dir = os.environ.get("PKGME_LOG_DIR", None)
> log_filename = 'pkgme.log'
> if log_dir is None:
> log_dir = os.path.expanduser(os.path.join('~', '.cache', 'pkgme'))
> return os.path.join(log_dir, log_filename)
>
> Which isn't superior (two calls to o.p.j), but I thought it might at least be interesting to share.

Yeah, I thought the same thing as I was typing it, so I changed the code
to what we both expected.

Please re-review.

Thanks,

James

[Jonathan Lange (jml)]

Thanks!

review: Approve

=== modified file 'pkgme/tests/test_trace.py'
--- pkgme/tests/test_trace.py	2011-07-29 14:37:35 +0000
+++ pkgme/tests/test_trace.py	2012-01-19 12:22:24 +0000
@@ -29,6 +29,14 @@
             os.path.join(temp_home.path, '.cache', 'pkgme', 'pkgme.log'),
             log_location)
 
+    def test_log_location_from_env_var(self):
+        # If PKGME_LOG_DIR is set in the environment we log there.
+        temp_home = self.with_temp_home()
+        temp_dir = self.useFixture(TempdirFixture())
+        self.useFixture(EnvironmentVariableFixture('PKGME_LOG_DIR', temp_dir.path))
+        log_location = trace.get_log_location()
+        self.assertEqual(os.path.join(temp_dir.path, 'pkgme.log'), log_location)
+
     def test_log_to_file(self):
         self.with_temp_log()
         trace.log("message")

=== modified file 'pkgme/trace.py'
--- pkgme/trace.py	2011-07-29 14:37:35 +0000
+++ pkgme/trace.py	2012-01-19 12:22:24 +0000
@@ -8,8 +8,11 @@
 
 def get_log_location():
     """Return the path that we're logging to."""
-    return os.path.expanduser(
-        os.path.join('~', '.cache', 'pkgme', 'pkgme.log'))
+    log_dir = os.environ.get("PKGME_LOG_DIR", None)
+    log_filename = 'pkgme.log'
+    if log_dir is None:
+        log_dir = os.path.expanduser(os.path.join('~', '.cache', 'pkgme'))
+    return os.path.join(log_dir, log_filename)
 
 
 class LoggingFixture(Fixture):