Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plain base Exception instances should not be raised #340

Open
yarikoptic opened this issue May 1, 2020 · 4 comments
Open

Plain base Exception instances should not be raised #340

yarikoptic opened this issue May 1, 2020 · 4 comments
Labels
category: bug errors in the code or code behavior

Comments

@yarikoptic
Copy link
Contributor

Ideally many exceptions raised by HDMF should have some base classe(s) for exceptions of "HDMF nature". For some cases where it is indeed a typical IndexError (e.g. out of bounds like below), should be ok to raise it, but even then there could be class HDMFIndexError(IndexError).

Why am I here with this -- I just ran into:

  File "/usr/lib/python3/dist-packages/hdmf/build/objectmapper.py", line 969, in construct
    raise Exception(msg) from ex
Exception: Could not construct DynamicTableRegion object due to The index 63 is out of range for this DynamicTable of length 63

and code indeed has a good number of those:

$> git grep 'raise Except'
src/hdmf/backends/hdf5/h5tools.py:                    raise Exception(msg) from exc
src/hdmf/backends/hdf5/h5tools.py:                raise Exception(msg) from exc
src/hdmf/backends/hdf5/h5tools.py:            raise Exception(msg) from exc
src/hdmf/backends/hdf5/h5tools.py:            raise Exception("Could not create dataset %s in %s" % (name, parent.name)) from exc
src/hdmf/backends/hdf5/h5tools.py:                raise Exception(msg) from exc
src/hdmf/backends/hdf5/h5tools.py:            raise Exception(msg) from exc
src/hdmf/build/objectmapper.py:                raise Exception(msg)
src/hdmf/build/objectmapper.py:                        raise Exception(msg) from ex
src/hdmf/build/objectmapper.py:                            raise Exception(msg) from ex
src/hdmf/build/objectmapper.py:                        raise Exception(msg) from ex
src/hdmf/build/objectmapper.py:                        raise Exception(msg) from ex
src/hdmf/build/objectmapper.py:            raise Exception(msg) from ex
src/hdmf/container.py:            raise Exception('cannot reassign container_source')
src/hdmf/data_utils.py:            raise Exception('Data type could not be determined. Please specify dtype in DataChunkIterator init.')
src/hdmf/spec/spec.py:            raise Exception('Cannot re-assign parent')
src/hdmf/testing/testcase.py:                        raise Exception(err)
src/hdmf/utils.py:                raise Exception('docval for {}: keys {} are not supported by docval'.format(a['name'],
src/hdmf/utils.py:                raise Exception(msg)
src/hdmf/utils.py:                    raise Exception(msg)
src/hdmf/utils.py:                    raise Exception(msg)
src/hdmf/utils.py:                    raise Exception(msg)
src/hdmf/utils.py:                    raise ExceptionType(msg)

which makes it hard to impossible for outside code to really tell if it is something "core" (permission, IO, etc errors) spit out by standard libraries or something PyNWB or HDMF dislikes. It makes outside code to resort to just except Exception which IIRC even pep8 doesn't like ;-)

@yarikoptic yarikoptic added the category: bug errors in the code or code behavior label May 1, 2020
@bendichter
Copy link
Contributor

Thanks @yarikoptic. This sounds valuable. We'd have to think about what we want our Exception ontology to be.

Anyone want to offer some suggestions?

@yarikoptic
Copy link
Contributor Author

"ontology" -- ;-) Exception ontologies like a religion and might cause if not a war, then a stomach to rumble to start with.
I would recommend to review your favorite established libraries, and follow the suit. Overall, reusing standard exceptions https://docs.python.org/3/library/exceptions.html is a good strategy to follow, and HDMF largely already uses them. It is for those Exception's originally mentioned -- some could be replaced with standard ones or some more HDMF specific exceptions could be defined.

@oruebel
Copy link
Contributor

oruebel commented Nov 3, 2020

I think especially in the ObjectMapping it will be useful to have our own exception types to more clearly communicate where the exception is coming from. In other cases it will be useful to user more specific standard exception types (e.g, when we raise issues due to HDF5).

@rly
Copy link
Contributor

rly commented Nov 4, 2020

See NeurodataWithoutBorders/pynwb#1144 for a need for a dedicated Exception class for missing namespaces.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: bug errors in the code or code behavior
Projects
None yet
Development

No branches or pull requests

4 participants