jazzband / tablib

Python Module for Tabular Datasets in XLS, CSV, JSON, YAML, &c.
https://tablib.readthedocs.io/
MIT License
4.59k stars 590 forks source link

Better exceptions #487

Closed nuno-andre closed 3 years ago

nuno-andre commented 3 years ago

It's best practise for libraries to have a common parent class for its custom exceptions so that they all can be easily catched.

This PR also refines the built-in exceptions inheritance and adds autodoc for the exceptions module.

codecov[bot] commented 3 years ago

Codecov Report

Merging #487 (a99753a) into master (630488d) will increase coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #487   +/-   ##
=======================================
  Coverage   90.66%   90.67%           
=======================================
  Files          28       28           
  Lines        2615     2616    +1     
=======================================
+ Hits         2371     2372    +1     
  Misses        244      244           
Impacted Files Coverage Δ
src/tablib/exceptions.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 630488d...a99753a. Read the comment docs.

hugovk commented 3 years ago

Will close and re-open to see if we get an auto docs build!

(https://github.com/jazzband-roadies/help/issues/211)

hugovk commented 3 years ago

It worked! Docs render properly:

https://tablib--487.org.readthedocs.build/en/487/api/#module-tablib.exceptions

Do we want to use the old exception doc descriptions as exception text, or use these new ones?

claudep commented 3 years ago

The new descriptions are better, IMHO, but they could be even better. E.g. Invalid size is too generic, we could explain a little bit more.

nuno-andre commented 3 years ago

Actually not new, I've only changed the strings to docstrings ;)

Maybe something like "You're trying to add something that doesn't quite look right: Only Datasets can be added to a DataBook"?

There were some missing exceptions in docs:

exception docstrings docs
InvalidDatasetType Only Datasets can be added to a DataBook. You're trying to add something that doesn't quite look right.
InvalidDimensions Invalid size. You're trying to add something that doesn't quite fit right.
InvalidDatasetIndex Outside of Dataset size.
HeadersNeeded Header parameter must be given when appending a column in this Dataset.
UnsupportedFormat Format is not supported. You're trying to add something that doesn't quite taste right.
TablibException Tablib common exception.
nuno-andre commented 3 years ago

@claudep I totally agree. Do you mean in code, where they're raised, or adding the message to exception? Something like:

class InvalidDimension(TablibException):
    '''A more specific message.
    '''
    def __init__(self, msg=None, *args, **kwargs):
        if not msg:
            msg = self.__doc__
        super().__init__(msg, *args, **kwargs)
claudep commented 3 years ago

No, I was only referring to the docstring. In the code, the message should be the best possible in the place where it is raised.

nuno-andre commented 3 years ago

E.g. The size of the column or row doesn't fit the table dimensions.?

Also, do you think it would be worth adding the previous snippet to TablibException so that a custom exception has an explicit message about the error if it's raised without it?

claudep commented 3 years ago

I'm not sure, if you find in the code examples where either the exception name or the exception message is not clear enough, then let's fix that in the code.

nuno-andre commented 3 years ago

Of all the custom exceptions, only some UnsupportedFormat are raised with a message. In addition, there are some built-ins that I think could be reraised as tablib exceptions (with proper error messages). It would probably be necessary to add some other custom execeptions.

A base exception is always a plus when adding a library as a dependency. Happy to contribute to this revision.

PS: btw, I just found that pandas is out of the tablib's import machinery.