Be a little more strict about what exceptions are caught.

[swails]

I've been burned many times by having a bare except clause vacuum up any old exceptions. In particular, your commit had the following code:

            try:
                logfile = open(opt.logfile, 'a')
                now = datetime.datetime.now()
                logfile.write('# Log started on %02d/%02d/%d [mm/dd/yyyy] at '
                              '%02d:%02d:%02d\n' % (now.month, now.day, now.year,
                                                 now.hour, now.minute, now.second))
                 parmed_commands.setlog(logfile)
                 close_log_file = True
            except:
                sys.stderr.write("Could not open logfile. Skip logging")

There are 4 different lines of code after opening the logfile that get executed, 3 of which could raise an exception. Someone could mask the datetime module with their own by accident, raising an AttributeError that gets caught as an apparent permissions error. I could accidentally change the parmed_commands.setlog API that would raise any of a number of errors.

Or maybe I use a binary file open function to open opt.logfile (maybe BZ2File or GzipFile) but forget to encode the logfile text. It works fine in Python 2, but then raises a TypeError due to the bytes/str split in Python 3 that appears to users as some kind of permissions error. All of these could mask what's really happening and make whatever bug that comes out of it very difficult to find.

There are times to use a bare except -- usually if you want to print something before re-raising the exception (I use one in ParmEd in the format registry metaclass for a very specific reason, but that one instance has caused problems like I described above). In general, though, you should execute as few functions in the try block as possible (use the try: ... except: ... else: block like I did here instead), and catch specific exceptions (can be multiple specific exception types). This will minimize the chances that you are masking errors and can make your life much easier when hunting certain bugs.

The only reason I bring this up and make a big deal of it is because I know you are writing your own program in pytraj, and adopting practices like the above has saved me lots of time compared to the time when I used to use bare excepts.

BTW, once you merge this PR into your branch, it will update to your PR and I can merge it. Thanks for the fix!

[hainm]

thanks @swails

Hai

[hainm]

@swails: you should always consider my PRs in the future are just my proposal PR. And I am willing to fix/learn. :P

[swails]

That's actually why I submitted a pr to your branch. I wanted to make sure you saw my thoughts and reasoning (whether or not you adopt my approach). I also wanted to make sure you were properly credited with the contribution.

Thanks!

[hainm]

thanks @swails for sharing your thought.

cheers
Hai

[hainm]

@swails ah, you're so right about using the bare except. I got this problem several times with pytraj. I got except raised but did not know what was really going on. (And then I needed to turn off the try ... except to know why. :D). thanks for the good practice.

And honestly I only knew the "deal" about using try and catching except few month ago. (and even asked you about using sys.stderr when introducing pytraj into amber).

Hai