Exception should Inhert from KegAppError or some base

[nZac]

Base exception:

keg/keg/app.py

Lines 23 to 24 in dba2cf4

	class KegAppError(Exception):
	pass

Config Exception:

keg/keg/config.py

Lines 18 to 19 in dba2cf4

	class ConfigurationError(Exception):
	pass

View Exceptions:

keg/keg/web.py

Lines 42 to 43 in dba2cf4

	class ViewArgumentError(Exception):
	pass

Asset Exceptions:

keg/keg/assets.py

Lines 12 to 13 in dba2cf4

	class AssetException(Exception):
	pass

[rsyring]

Why do these need to inherit from some base? That is usually helpful for something like Requests where you might actually want to catch a base exception b/c you don't care about the reason a request failed, just that it failed.

In the case of these exceptions, they are all distinct. I can't image ever using a construct like:

try:
   ....
except KegBaseError as e:
    # do something regardless of which Keg error it is

[nZac]

I would pose the reverse question... why wouldn't you want all the exceptions in a library to inherit from a common base? The base exception is 2 lines and to my understanding adds no overhead (maybe a heap allocation?) and no complexity other than "hey, inherit from the base class" if we add a new exception type. I would move all the exceptions into their own module too keg.exc, then it is obvious what should happen. Seem like only upsides to me... 🤷‍♂

Regardless my usecase is, flask has exception handlers that you can attach to the app and it would be helpful to catch all KegBaseErrors in case you don't handle a particular exception manually.

Also, as the Keg ecosystem expands, we can inject additional data into a KegBaseError if we inherit from it and send that data to sentry. This would be userful in KegElements for example when a template has issues to automatically inject debug information into the exception for Sentry to report. Being able to register the exceptions with Sentry for special handling is helpful.

Finally, it seems to be a reasonable practice across the community. This is how SQLAlchmey, Requests, Coverage does it... I'm sure there is more but that is the short list.

[rsyring]

I would pose the reverse question... why wouldn't you want all the exceptions in a library to inherit from a common base?

I don't really care that much, I just don't think it adds any benefit to this type of library and it encourages the handling "all errors" as if they were the same thing/type when they are all very different.

For example:

Regardless my usecase is, flask has exception handlers that you can attach to the app and it would be helpful to catch all KegBaseErrors in case you don't handle a particular exception manually.

This smells to me. I just don't see a use case where treating all these exception the same is going to be the best architecture. If you aren't going to handle them explicitly (assuming that's what you mean by "manually") then IMO the exception should probably bubble to the surface instead of being caught in some KegBaseError catch-all statement. But, I guess if you have the case that proves the exception (no pun intended), so be it.

Also, as the Keg ecosystem expands, we can inject additional data into a KegBaseError if we inherit from it and send that data to sentry. This would be userful in KegElements for example when a template has issues to automatically inject debug information into the exception for Sentry to report. Being able to register the exceptions with Sentry for special handling is helpful.

IMO YAGNI.

This is how SQLAlchmey, Requests, Coverage does it... I'm sure there is more but that is the short list.

Yes, but those examples all have true exception hierarchies. Where catching one high-level exception would be a good practice and good architecture in many common cases. In Keg's case, with the exceptions we throw, there isn't really a hierarchy and I currently believe catching them all with some generic handler is likely an anti-pattern.

I would move all the exceptions into their own module too keg.exc

I deliberately did not do that for Keg because the exceptions were disparate and usage in the library was confined to their module. Unlike something like SQLAlchemy where you may have one exception used by a bunch of different code. Then it makes sense to move them all to a centralized exceptions module.

---

I don't really care if you do the inheritance thing. I'm a -0 on it b/c it encourages a catch-all approach which doesn't currently make sense to me and feels like an anti-pattern. If you are a +1 then feel to move ahead.

I'd be a -1 on moving the exceptions to a centralized module for Keg, although I do appreciate that pattern and have used it elsewhere when it seemed the better approach. Just doesn't feel to me like it applies here. If you are a +1 here, probably best to get Matt's opinion and let his vote be the tie breaker.

Thanks for your thoughts.