3
\$\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\$
3
  • 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, 2017 at 8:07
  • 1
    \$\begingroup\$ Why not use Python's own csv module: devdocs.io/python~2.7/library/csv \$\endgroup\$
    – hjpotter92
    Aug 14, 2017 at 8:10
  • 1
    \$\begingroup\$ Corrected message. \$\endgroup\$
    – gogasca
    Aug 14, 2017 at 16:21

2 Answers 2

4
\$\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\$
2
\$\begingroup\$

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

\$\endgroup\$
5
  • \$\begingroup\$ what if you want to handle them to provide more user friendly errors ? \$\endgroup\$ Mar 18, 2022 at 18:48
  • \$\begingroup\$ In that case, handling the exception in this part of the code would still be wrong. The job of this code snippet is to report any errors back to the caller. The caller can then decide how to handle any exceptions, be they IOError or anything else. The code from the original post hides the actual error from the caller since it just returns None in case of an error. \$\endgroup\$ Mar 19, 2022 at 21:15
  • \$\begingroup\$ that is your point of view, because in the original question it does not say that, he wants to handle errors, not necessarily report them in a return. the errors could be logged to a file and later used to display them to the user. \$\endgroup\$ Mar 20, 2022 at 2:14
  • \$\begingroup\$ That's not only my point of view, that is common practice and straight-forward error handling. Instead, what you suggest, first logging into a file and then reading the log data back to reconstruct a user-targeting error message from that is so unusual that it almost qualifies as obfuscation. \$\endgroup\$ Mar 20, 2022 at 14:30
  • \$\begingroup\$ I know is a common patter, that is not my point, the fact of being common it does not means that is what the OP wanted specially since he/she did not mentioned that behavior \$\endgroup\$ Mar 20, 2022 at 14:35

Your Answer

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

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