kalekundert / byoc

MIT License
0 stars 0 forks source link

Better error handling #24

Closed kalekundert closed 2 years ago

kalekundert commented 3 years ago

appcli.param should provide a way to specify what error should get raised if an attribute can't be found. I'll have to think about this a little bit, because right now there is a pretty baked-in assumption that an AttributeError will be raised. But ultimately I need a way to make customized error messages (e.g. preset "xxx" not found, did you mean "xyz"?)

Edit: for the specific case of the "preset not found" error mentioned above, this would have to be implemented at the PresetConfig/PresetLayer level. I don't think it's actually that relevant to this discussion.

kalekundert commented 2 years ago

Here's how exceptions work currently:

This doesn't seem great: I can imagine wanting pick=list to return an empty list, but that's just impossible right now. The only way to make that possible is to make pick responsible for raising an exception if it doesn't like the generator it was given.


Right now I have two kinds of errors:

I'm starting to think this isn't a good distinction. ScriptError is used for programming logic errors, e.g. missing or redundant arguments. But ConfigError is also frequently triggered by programming errors.

Instead, I'm thinking it might be better to think of ConfigError as specifically meaning "no values could not be found". This is pretty much the same meaning as regular AttributeError, but I should still have my own subclass. If there are any current uses of ConfigError that don't fall under this umbrella, they should be replaced with a new exception class. The motivation for this is that this more narrow definition is something that frequently need to be reacted to, e.g. skip if no value is found.

All ConfigError uses:

Also, the names ConfigError and ScriptError are awful. Especially ConfigError, since there are a bunch of user-facing Config classes that have nothing to do with it. NoValueFound is definitely better. ApiError might be better than ScriptError.

kalekundert commented 2 years ago

Having thought about this a bit, I think the best philosophy is just to put the user in control of which exceptions get raised. This is already mostly the case: most of the exceptions that can get raised happen in user-provided callbacks.

The way to put the user in control of the NoValueFound exception is to have it raised by the pick function, which is user provided. This also resolves the weirdness where pick=list can't actually return an empty list. The problem this introduces is that I want the default exception to include a log describing the attempts to find a value. This log then needs to be made available to the pick function. It occurred to me that the best way to do this is to make the values iterator a custom object, which can have a log attribute. It could also grow methods to iterate metadata, see #26. This custom iterator object will simply iterate through the available values via the default iterator interface, so it will be compatible with any pick function. But it will also make more sophisticated information to pickers that are meant for use with appcli.

I should also modify the Method/Func getters to accept an argument describing which exception types should be treated as "missing values". By default this would just be NoValueFound, but could be anything. I might also want to give Key a similar argument, although that one would default to KeyError.

Edit: Method/Func already have such an argument.

kalekundert commented 2 years ago

One complication of allowing user-specified exceptions is that I can't rely on tidyexc anymore. Instead I need to make sure that the log has everything that the exception should display, and rely on that.