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

Better exceptions #487

Merged
merged 2 commits into from
Dec 11, 2020
Merged

Better exceptions #487

merged 2 commits into from
Dec 11, 2020

Conversation

nuno-andre
Copy link
Contributor

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
Copy link

codecov bot commented Dec 9, 2020

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
Copy link
Member

hugovk commented Dec 9, 2020

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

(jazzband/help#211)

@hugovk hugovk closed this Dec 9, 2020
@hugovk hugovk reopened this Dec 9, 2020
@hugovk
Copy link
Member

hugovk commented Dec 9, 2020

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
Copy link
Contributor

claudep commented Dec 9, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Contributor

claudep commented Dec 9, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor

claudep commented Dec 9, 2020

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
Copy link
Contributor Author

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.

Copy link
Contributor

@claudep claudep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good.

@claudep claudep merged commit 7035d79 into jazzband:master Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants