5
\$\begingroup\$

I'm writing a simple script which will search a source directory for certain types of files and move them to a destination directory. I'm using it as an opportunity to practice writing some "defensive" code which nicely reports errors to the user, rather than spitting out a stack trace.

As a start, the program should read a config file to determine the source and destination directories. There are many things that could go wrong here: the config file could be missing, have bad permissions, be invalid JSON, omit required fields, or have values of the wrong type. I'd like to anticipate these errors and print a helpful error message to the user while logging a more detailed traceback to a file.

I don't write much user-facing code in my day-to-day work, so I'm not familiar with Python idioms for error reporting. I have a vague idea that I'd like to keep the frontend (which consists of doing the error checking and reporting) separate from the backend. Below is my stab at it:

import collections
import json
import logging
import pathlib
import sys

# Logging #####################################################################

class StreamFormatter(logging.Formatter):
    def format(self, record):
        """Squash exception information."""
        if record.levelno == logging.ERROR:
            return "Error: " + record.message
        else:
            return record.message

logger = logging.getLogger(__name__)

# create a log file handler with detailed tracebacks
file_handler = logging.FileHandler("debug.log")
formatter = logging.Formatter("%(asctime)s - %(name)s - %(levelname)s - "
                              "%(message)s")
file_handler.setFormatter(formatter)

# create a stream handler with no exception info, just prints messages
stream_handler = logging.StreamHandler()
stream_handler.setFormatter(StreamFormatter())

logger.addHandler(file_handler)
logger.addHandler(stream_handler)

###############################################################################

def main():
    config = _logged_load_config("config.json")
    _logged_validate_config(config)

    destinations = _logged_find_destinations(config["dest"])

    print(config)

# Config Validation ###########################################################

Check = collections.namedtuple("Check", ["check", "message"])

def is_directory(x):
    try:
        return pathlib.Path(x).is_dir()
    except TypeError:
        return False

directory_check = Check(is_directory, "Must be a an existing directory.")

REQUIRED_CONFIG = {
    "dest": [directory_check],
    "source": [directory_check]
    }

# Logged Helper Functions #####################################################

def _logged_load_config(path):
    try:
        f = open(path)
    except IOError:
        logger.exception("Cannot open config file.")
        sys.exit(1)

    try:
        config = json.load(f)
    except ValueError:
        logger.exception("JSON is malformed.")
        sys.exit(1)
    finally:
        f.close()

    return config

def _logged_validate_config(config):
    missing = REQUIRED_CONFIG.keys() - config.keys()
    if missing:
        logger.error("Missing the following required keys: {}".format(missing))
        sys.exit(1)

    is_valid, key, rule = validate_config(config)
    if not is_valid:
        logger.error("The key '{}' is not valid: {}".format(key, rule.message))
        sys.exit(1)

def _logged_find_destinations(dest):
    try:
        return find_destination_dirs(dest)
    except IOError:
        logger.exception("Couldn't read destination directory.")
        sys.exit(1)

# Library Functions ###########################################################

def validate_config(config):
    for key, rules in REQUIRED_CONFIG.items():
        for rule in rules:
            if not rule.check(config[key]):
                return (False, key, rule)
    else:
        return (True, None, None)

def find_destination_dirs(path):
    path = pathlib.Path(path)
    return [x for x in path.iterdir() if x.is_dir()]

if __name__ == "__main__":
    main()

In this code, I've made a serious effort to keep main() as clean as possible. I want to be able to open this script a year from now and get a good idea of what is going on by glancing at the contents of main. That means moving all of the clutter of logging and error checking to _logging_* functions with self-describing names. I actually quite like this layout. Is this something that's considered good practice?

Another thing I've tried to do is to factor out functionality that I might want to reuse into separate "pure" functions which do no logging, and do not cause the process to exit on error. For example, I might want to use validate_config in another module, and I definitely do not want it calling sys.exit if the config isn't valid. Instead, I put the logging and error-checking in _logged_validate_config, which should be "private" to the module. Granted, I don't have many "backend" functions here -- if I did, I'd toss them in a module.

I've got a module-level logger, even though it's only used by the _logging_* functions. Should I instead create a logger instance in the body of main() and pass it to the utility functions as an argument?

Lastly: When I want to see examples of good, idiomatic library code, I usually look through Python's standard library source. What are some open source projects that I might look to in order to find good examples of user-facing code? That is, code that performs input validation, error handling, logging, etc...

\$\endgroup\$
1
  • 2
    \$\begingroup\$ This sentence is worth a million bucks: I want to be able to open this script a year from now and get a good idea of what is going on. Welcome to Code Review! \$\endgroup\$
    – Phrancis
    Feb 10, 2015 at 5:20

2 Answers 2

1
\$\begingroup\$

The log messages are not informative enough. For example, "Cannot open config file." — which file are you talking about, and why didn't you include the path in the message?

By convention, main() should be defined just before the end of the file (though some programmers prefer it at the beginning). Putting it somewhere in the middle makes it hard to find.

The main theme that I see is that you seem to treat exceptions more as an annoyance than as a tool for directing flow of control and relaying information.

Sprinkling sys.exit() calls everywhere looks like a headache. You should be better off raising or re-raising exceptions after logging the error, and having just one sys.exit(1) in main(). For example:

def _logged_load_config(path):
    try:
        with open(path) as f:
            return json.load(f)
    except IOError:
        logger.exception("Cannot open config file %s" % (path))
        raise
    except ValueError:
        logger.exception("JSON is malformed.")
        raise

Likewise, validate_config() could also make better use of exceptions, especially since it is designed to stop looking further at the first error it encounters. Instead of returning a (False, key, rule) tuple, throw a custom exception that conveys the key and rule. Instead of returning (True, None, None), just let the function return naturally.

I suspect that the logging may be getting in the way of good design. As general advice, I would suggest that you temporarily rip out all of the logging code, reorganize the remaining code, then reconsider where to add logging back in strategically.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks! See, I (sort of dogmatically) stick to the idea that try: blocks should wrap as little code as possible, when that's not always necessary. Would the following be good practice? I define a custom exception MyException for anticipated errors. In _logged_load_config, etc, I re-raise the exceptions as MyException (keeping the stack trace). I then wrap the call to main() with try: main(); except MyException: # nicely log the error; except: # log a fatal error I didn't expect!. \$\endgroup\$
    – jme
    Feb 10, 2015 at 14:46
0
\$\begingroup\$

One thing after a quick look through: validate_config either returns (False, key, rule) or (True, None, None). So the initial boolean is not necessary - whether there is an error is determined by whether the second and third elements are not None. You might want to rename that function as well, what it does is really just return the first configuration error.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.