1
\$\begingroup\$

Trying to improve my function, as will be used by most of my code. I'm handling most common exception (IOError) and handling when data has no values.

READ_MODE = 'r'


def _ReadCsv(filename):
  """Read CSV file from remote path.

  Args:
    filename(str): filename to read.
  Returns:
    The contents of CSV file.
  Raises:
    ValueError: Unable to read file
  """
  try:
    with open(filename, READ_MODE) as input_file:
      data = input_file.read()
      if not data:
        raise ValueError('No data available')
  except IOError as e:
    logging.exception(e)
  return data
\$\endgroup\$
  • 4
    \$\begingroup\$ It's incorrect to to say you were "Unable to read file" when you successfully read an empty file. \$\endgroup\$ – Peilonrayz Aug 14 '17 at 8:07
  • 1
    \$\begingroup\$ Why not use Python's own csv module: devdocs.io/python~2.7/library/csv \$\endgroup\$ – hjpotter92 Aug 14 '17 at 8:10
  • 1
    \$\begingroup\$ Corrected message. \$\endgroup\$ – user260826 Aug 14 '17 at 16:21
2
\$\begingroup\$

The only purpose of using a try/except that does not re-raise would be if it doesn't matter if there's an IOError. However, in this case, if an IOError occurs, it will be logged, then ignored and then return is going to raise a NameError - that 'data' is not defined. Since you are raising an error on empty results, I assume you handle that in the caller or want it to stop the process. In either case, I'd do something like

import logging
READ_MODE = 'r'


def _ReadCsv(filename):
    """Read CSV file from remote path.

    Args:
      filename(str): filename to read.
    Returns:
      The contents of CSV file.
    Raises:
      ValueError: Unable to read file
    """
    data = None
    try:
        with open(filename) as fobj:
            data = fobj.read()
    except IOError:
        logging.exception('')
    if not data:
        raise ValueError('No data available')
    return data

Also, there's no need for the with open construct if you are just reading it. With open is preferred if you do more than one thing with the file object, so it gets closed properly. By not assigning the file open to a variable, it will get closed properly and garbage collected. Keep the with open, since garbage collection is an implementation detail you should not rely on any particular behavior.

With logging.exception, all the neat stuff that you see people doing manually is already taken care of. By simply calling it with an empty string, you get the full traceback, exception type and text without doing anything else.

logging.exception('')

Is equivalent to

logging.error(''.join(traceback.format_exception(*sys.exc_info)))

or

logging.error('', exc_info=True)
\$\endgroup\$
3
\$\begingroup\$

Don't handle the exception in this part of the code, since it will hide errors. Just let it bubble up.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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