Path: csiph.com!fu-berlin.de!uni-berlin.de!not-for-mail
From: Ian Kelly <ian.g.kelly@gmail.com>
Newsgroups: comp.lang.python
Subject: Re: common mistakes in this simple program
Date: Mon, 29 Feb 2016 09:29:21 -0700
Lines: 92
Message-ID: <mailman.21.1456763409.20602.python-list@python.org>
References: <CACT3xuWUk02Or78XHTdkFLsy6CMVHEYKOyid3hFJxw8Wh6-g1A@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
In-Reply-To: <CACT3xuWUk02Or78XHTdkFLsy6CMVHEYKOyid3hFJxw8Wh6-g1A@mail.gmail.com>
Precedence: list
Xref: csiph.com comp.lang.python:103729

On Mon, Feb 29, 2016 at 8:18 AM, Ganesh Pal <ganesh1pal@gmail.com> wrote:
> Iam on python 2.6

Python 2.6 has been unsupported since October 2013. Among other
things, that means it is no longer receiving security updates like
more recent versions. Unless you have an extremely strong reason for
wanting to stay to Python 2.6, you should update your Python version.

> 1. usage of try- expect

try-except in every single function is a code smell. You should only
be using it where you're actually going to handle the exception. If
you catch an exception just to log it, you generally should also
reraise it so that something further up the call chain has the
opportunity to handle it.

> 2. Return of True/ False

If you're returning True to mean "This completed without exception"
and False to mean "This raised an exception", then the more Pythonic
thing to do would just be to let the exception propagate, giving the
caller the opportunity to catch, inspect, and handle the exception.

> """
> """

An empty docstring is pointless.

> import os
> import shlex
> import subprocess
> import sys
> import time
> import logging
> import run
> import pdb

Most of these are unused. Only import modules that your code is actually using.

> def run_cmd_and_verify(cmd, timeout=1000):
>     try:
>         pdb.set_trace()
>         out, err, ret = run(cmd, timeout=timeout)

What is "run"? It's imported like a module above, but here you're
using it like a function.

>         assert ret ==0,"ERROR (ret %d): " \
>                 " \nout: %s\nerr: %s\n" % (ret, out, err)
>     except Exception as e:
>         print("Failed to run %s got %s" % (cmd, e))
>         return False
>     return True
>
> def prep_host():
>     """
>     Prepare clustering
>     """
>     for cmd in ["ls -al",
>                 "touch /tmp/file1",
>                 "mkdir /tmp/dir1"]:
>         try:
>             if not run_cmd_and_verify(cmd, timeout=3600):
>                 return False
>         except:

What exceptions are you expecting this to catch? run_cmd_and_verify
already catches any expected exceptions that it raises.

Also, you should almost never use a bare except like this. Use "except
Exception" instead. A bare except will catch things like
KeyboardInterrupt and SystemExit that should not be caught.

>               print("Error: While preparing cluster !!!")
>               return False
>     print("Preparing Cluster.....Done !!!")
>     return True
>
>
> def main():
>     functions = [prep_host]
>     for func in functions:
>         try:
>             func()
>         except Exception as e:

As above, what exceptions are you expecting here?

>             print(e)
>             return False
> if __name__ == '__main__':
>     main()