Merge lp:~bryce/apport/omit_empty into lp:~apport-hackers/apport/trunk

[Bryce Harrington]

Fixes a couple typos

Fixes 813798 by adding 'omit_empty' flag to attach_root_command_outputs() so it doesn't append empty fields when a command gives no output.

[Martin Pitt (pitti)]

Thanks for the typo fixes!

As for the attach_root_command_outputs() fix, I feel that it would make more sense to just always skip empty keys. Also, I think we should always strip() the result, leading and trailing empty lines have little information (I'm not aware of apport hooks that attach brainfuck source :) ).

So WDYT about this:

   buf = f.read.strip()
   if not buf:
       continue

?

review: Needs Information

[Bryce Harrington (bryce)]

On Thu, Jul 21, 2011 at 04:32:33AM -0000, Martin Pitt wrote:
> Review: Needs Information
> Thanks for the typo fixes!
>
> As for the attach_root_command_outputs() fix, I feel that it would make more sense to just always skip empty keys. Also, I think we should always strip() the result, leading and trailing empty lines have little information (I'm not aware of apport hooks that attach brainfuck source :) ).
>
> So WDYT about this:
>
> buf = f.read.strip()
> if not buf:
> continue
>
> ?

Yep, that would actually be my preference. Just wasn't sure whether you
wanted to keep the empties for some reason.

Bryce

[lp:~bryce/apport/omit_empty]

1921. By Bryce Harrington on 2011-07-21

Review by pitti: Just always strip and always skip empties.

[Martin Pitt (pitti)]

Thanks!

review: Approve

=== modified file 'apport/hookutils.py'
--- apport/hookutils.py	2011-07-15 17:23:19 +0000
+++ apport/hookutils.py	2011-07-21 05:32:26 +0000
@@ -32,7 +32,7 @@
 def path_to_key(path):
     '''Generate a valid report key name from a file path.
         
-    This will meet apport's restrictions on the characters used in keys.
+    This will replace invalid punctuation symbols with valid ones.
     '''
     return path.translate(_path_key_trans)
 
@@ -54,7 +54,7 @@
 def read_file(path):
     '''Return the contents of the specified path. 
         
-    Upon error, this will deliver a a text representation of the error,
+    Upon error, this will deliver a text representation of the error,
     instead of failing.
     '''
     try:
@@ -83,7 +83,7 @@
 def attach_dmesg(report):
     '''Attach information from the kernel ring buffer (dmesg).
 
-    This won't overwite already existing information.
+    This will not overwrite already existing information.
     '''
     try:
         if not report.get('BootDmesg', '').strip():
@@ -328,7 +328,9 @@
         # now read back the individual outputs
         for keyname in command_map:
             f = open(os.path.join(workdir, keyname))
-            report[keyname] = f.read()
+            buf = f.read().strip()
+            if buf:
+                report[keyname] = buf
             f.close()
     finally:
         shutil.rmtree(workdir)