Tukukkk

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

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)

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