dropbox / pygerduty

A Python library for PagerDuty.
MIT License
163 stars 72 forks source link

Consolidate exceptions #78

Closed amckenna-pinterest closed 5 years ago

amckenna-pinterest commented 5 years ago

I was troubleshooting a bug today in our code and noticed that these exceptions are defined in multiple places. I was attempting to catch pygerduty.v2.BadRequest and was getting pygerduty.common.BadRequest instead.

These can all be defined in one place and imported. That way, when you throw the exception it is always an instance of the same class.

Thanks!

gmjosack commented 5 years ago

One thing to be aware of here is this isn't backwards compatible with code catching exceptions from the old locations. For compatibility you might want to re-export (with a comment why) from the old locations unless there's no desire to maintain backwards compat.

amckenna-pinterest commented 5 years ago

I don't believe that is necessary:

Python 3.7.2 (default, Feb 12 2019, 08:15:36)
[Clang 10.0.0 (clang-1000.11.45.5)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import pygerduty
>>> import pygerduty.v2
>>> pygerduty.v2.Error == pygerduty.exceptions.Error
True
>>> import pygerduty.common
>>> pygerduty.common.BadRequest == pygerduty.exceptions.BadRequest
True
gmjosack commented 5 years ago

That's only true for classes being imported into the old modules but BadRequest and NotFound or no longer in the v2 module for example as they're not being imported there.

Before:

>>> from pygerduty.v2 import BadRequest
>>> 

After:

>>> from pygerduty.v2 import BadRequest
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: cannot import name BadRequest
amckenna-pinterest commented 5 years ago

OK, I see what you mean. But those exceptions are also not raised anywhere in those modules. So anyone using them before was likely using the wrong instance anyway.

amckenna-pinterest commented 5 years ago

I guess it's better to make existing code work again rather than break it, though. I'll update the PR.

amckenna-pinterest commented 5 years ago

This should be good to go now! Thanks.

cugini-dbx commented 5 years ago

This makes a lot of sense, thanks @amckenna-pinterest !