Merge lp:~javier.collado/utah/static_analysis_6 into lp:utah

[Javier Collado]

Static analysis changes. As usual every commit fixes a different problem/warning.

[Joe Talbott (joetalbott)]

Looks good. Can you give some details on the tool you're using so we can check our code before submitting?

Merge away.

review: Approve

[Javier Collado (javier.collado)]

@Joe

What I use is flake8 (pyflakes + pep8 together):
http://pypi.python.org/pypi/flake8/.

It isn't packaged in ubuntu so you have to install it with:
pip install flake8

In the branch directory I run a command like this:
find . -name '*.py' | xargs flake8

Lately pep8 (http://pypi.python.org/pypi/pep8) has been updated with additional warning/error codes, so sometimes I also run pep8 since latest changes aren't included in flake8 yet. Anyway, I'm not convinced about the usefulness of some of the new warnings/errors, so I'm not being strict with them.

Another interesting tool is autopep8 (http://pypi.python.org/pypi/autopep8/0.8.1) which applies changes to source files needed to meet pep8. For now, I'm just running some experiments with it.

Anyway, the most important thing is to integrate static analysis with your editor, so that any time something is wrong you get immediate feedback and fix it because otherwise all the highlighting becomes annoying. In my case, I'm using vim and syntastic (http://www.vim.org/scripts/script.php?script_id=2736) configured to use flake8. If you use another editor, is quite likely that some kind of integration is already available for it.

For the future, another tool I'd like to use is pep257 (http://pypi.python.org/pypi/pep257/) to make sure that docstrings have a common format across the whole project. However, it currently returns around 1300 errors, so that's something I won't be attempting any time soon.

=== modified file 'client.py'
--- client.py	2012-09-14 11:11:38 +0000
+++ client.py	2012-10-03 09:37:24 +0000
@@ -41,7 +41,8 @@
     parser.add_argument('-i', '--install-type',
                         choices=['desktop', 'server', 'mini', 'alternate'],
                         default='desktop',
-                        help='Installation Variant (i.e. server, desktop, etc.)')
+                        help=('Installation Variant '
+                              '(i.e. server, desktop, etc.)'))
     parser.add_argument('-r', '--runlist', type=url_argument,
                         # Having a default here means that if a resume happens
                         # post-reboot without a runlist specified, it will
 
=== modified file 'utah/client/common.py'
--- utah/client/common.py	2012-09-27 13:55:41 +0000
+++ utah/client/common.py	2012-10-03 09:37:24 +0000
@@ -68,7 +68,9 @@
         cmd_prefix = "sudo -n -u {} ".format(run_as)
     else:
         cmd_prefix = ""
-        run_as = getpass.getuser() # just a guess, but we're only displaying it
+
+        # just a guess, but we're only displaying it
+        run_as = getpass.getuser()
 
     if command is None:
         return
@@ -80,8 +82,9 @@
         raise TimeoutAlarm
 
     start_time = datetime.datetime.now()
-    p = subprocess.Popen("{} {}".format(cmd_prefix, command), shell=True, cwd=cwd, stdout=subprocess.PIPE,
-                         stderr=subprocess.PIPE)
+    p = subprocess.Popen("{} {}".format(cmd_prefix, command),
+                         shell=True, cwd=cwd,
+                         stdout=subprocess.PIPE, stderr=subprocess.PIPE)
 
     if timeout != 0:
         signal.signal(signal.SIGALRM, alarm_handler)
 
=== modified file 'utah/client/phoenix.py'
--- utah/client/phoenix.py	2012-09-26 15:23:11 +0000
+++ utah/client/phoenix.py	2012-10-03 09:37:24 +0000
@@ -3,17 +3,16 @@
 
 import argparse
 import os
-import sys
-
-TC_CONTROL_NAME='tc_control'
-TS_CONTROL_NAME='ts_control'
-TSLIST_NAME='tslist.run'
-MASTER_NAME='master.run'
-CHANGEME="CHANGE_ME"
+
+TC_CONTROL_NAME = 'tc_control'
+TS_CONTROL_NAME = 'ts_control'
+TSLIST_NAME = 'tslist.run'
+MASTER_NAME = 'master.run'
+CHANGEME = "CHANGE_ME"
+
 
 class Phoenix(object):
 
-
     def __init__(self, testsuite, directory='.', testcases=['testcase1']):
         self.testsuite = testsuite
         self.directory = directory
@@ -60,7 +59,8 @@
             self.FILE_CONTENT[TSLIST_NAME] += "- test: {name}\n".format(name=d)
 
         self.FILE_CONTENT[MASTER_NAME] += "  - name: {}\n".format(testsuite)
-        self.FILE_CONTENT[MASTER_NAME] += "    fetch_cmd: {}\n".format(CHANGEME)
+        self.FILE_CONTENT[MASTER_NAME] += \
+                "    fetch_cmd: {}\n".format(CHANGEME)
 
         self.debug()
 
@@ -119,7 +119,7 @@
             help='names of testcases to create')
     parser.add_argument('--directory', '-D', dest='directory', default='.',
             help='where to create the testsuite')
-    
+
     args = parser.parse_args()
 
     phoenix = Phoenix(args.testsuite[0],
 
=== modified file 'utah/client/tests/test_phoenix.py'
--- utah/client/tests/test_phoenix.py	2012-09-25 19:21:16 +0000
+++ utah/client/tests/test_phoenix.py	2012-10-03 09:37:24 +0000
@@ -6,14 +6,17 @@
 
 directory_name = "/tmp/phoenix"
 
+
 def setUp():
     if os.path.exists(directory_name):
         raise Exception("directory '{}' already exists".format(directory_name))
 
+
 def tearDown():
     if os.path.exists(directory_name):
         shutil.rmtree(directory_name)
 
+
 class TestPhoenix(unittest.TestCase):
     testsuite_name = "testsuite_name"
     testcases = ['test_one', 'test_two']
@@ -35,7 +38,6 @@
                 Phoenix,
                 )
 
-
     def test_providing_required_args(self):
         """
         Test that __init__ succeeds with the required arguments.
@@ -43,7 +45,6 @@
 
         self.assertTrue(self.phoenix is not None)
 
-
     def test_suite_name(self):
         """
         Test that the testsuite name is correctly set.
@@ -78,7 +79,7 @@
         """
         Test that the default testcase is correctly set.
         """
-        
+
         phoenix = Phoenix(self.testsuite_name)
         # there should be only one default testcase.
         self.assertEqual(len(phoenix.testcases), 1)
 
=== modified file 'utah/client/tests/test_runner.py'
--- utah/client/tests/test_runner.py	2012-08-29 18:22:44 +0000
+++ utah/client/tests/test_runner.py	2012-10-03 09:37:24 +0000
@@ -20,7 +20,8 @@
         self.result_class = ResultYAML
         self.state_file = '/tmp/state.yaml'
         self.state_agent = StateAgentYAML(state_file=self.state_file)
-        self.runner = Runner(install_type='desktop', result_class=self.result_class,
+        self.runner = Runner(install_type='desktop',
+                             result_class=self.result_class,
                              state_agent=self.state_agent)
         self.bad_dir = "/tmp/does_not_exist"
 
@@ -123,7 +124,9 @@
         fp.write(master_runlist_content)
         fp.close()
 
-        runner = Runner(install_type='desktop', result_class=self.result_class, runlist=runlist)
+        runner = Runner(install_type='desktop',
+                        result_class=self.result_class,
+                        runlist=runlist)
         runner.save_state()
 
         print("tests: {}".format(runner.count_tests()))
 
=== modified file 'utah/client/tests/test_state_agent.py'
--- utah/client/tests/test_state_agent.py	2012-09-06 15:44:36 +0000
+++ utah/client/tests/test_state_agent.py	2012-10-03 09:37:24 +0000
@@ -159,7 +159,8 @@
             shutil.copytree(test_src, test_dir)
 
         state_agent = StateAgentYAML(state_file=self.state_file)
-        runner = Runner(install_type="desktop", state_agent=state_agent, resume=True)
+        runner = Runner(install_type="desktop", state_agent=state_agent,
+                        resume=True)
         runner.load_state()
 
         self.assertEqual(runner.status, 'RUN')
 
=== modified file 'utah/provisioning/baremetal/cobbler.py'
--- utah/provisioning/baremetal/cobbler.py	2012-09-19 19:03:03 +0000
+++ utah/provisioning/baremetal/cobbler.py	2012-10-03 09:37:24 +0000
@@ -173,8 +173,9 @@
         users, remote login, not sudo, etc.
         """
         if self._runargs(['sudo', 'cobbler'] + cmd) != 0:
-            self.logger.error('Cobbler command failed: ' + ' '.join(cmd))
-            raise UTAHBMProvisioningException('Cobbler command failed: ' + ' '.join(cmd))
+            error_msg = 'Cobbler command failed: ' + ' '.join(cmd)
+            self.logger.error(error_msg)
+            raise UTAHBMProvisioningException(error_msg)
 
     def _start(self):
         """