Draft: Parameter value exceptions

assigned to @luenensc

@luenensc before you put much more time into this i'd like to talk about this MR..

i understand your effort to make errors uniform, but from my idea of coding, this is not the way to go. i have multiple points i would like to talk about.

• Error handling should be dead simple
• IMO the wording is quite difficult to read. in general its easier to understand positive names like isOffsetStr than noOffsetStr
• IMO returns should be simple like isOffsetStr(obj: Any) -> bool
• Error messages should be generated raised where the error occur, because it helps to understand the code
• custom Exceptions should have no special syntax if not explicitly needed. this means a new have no body, like class InvalidParam(ValueError): pass
• some more

I know this sounds harsch, but IMO our current goal is to simplify and reduce the code and the complexity of saqc. More complex error handling itself is error prone, because one never can be sure if the error really came from the feature or the handling itself..

Sorry i hope i did not used to hard words, i just dont want you to put to much effort in something that might be rejected. So i like to open this discussion. It might be good to do this in real life ?

@schaefed you invited also

🙏 b.

okaaaaay: hm. . main goal was to minimizes big if-statements-code blocks, that would clutter the functions bodies. Of course those would be complexity minimal, but @schaefed also kinda aprooved of that attempt in another MR. In the sense, that it would be a good way to have something like:

if checkFunc(val):
   raiseError(paraname)

IMO my implementation is a redundancy-minimal way to achieve this.

IMO the wording is quite difficult to read. in general its easier to understand positive names like isOffsetStr than noOffsetStr

okay, hm. but i dont find that so difficult to read: its not instead of is. Its also in the logic of the check: raise alarm if value is not as intended.-> otherwise youd have to add negations else were syntactically.

IMO returns should be simple like isOffsetStr(obj: Any) -> bool

Yes that would be even more basic, but Strings are returned so that you dont have that much redundancy between checking and erromessage generation. Since you can also if - check strings, its not that much different.

custom Exceptions should have no special syntax if not explicitly needed. this means a new have no body, like class InvalidParam(ValueError): pass

Sorry thats a left over. I only use ValueError with errormessages constructed while checking value validity. the exceptions module is an artifact.

because one never can be sure if the error really came from the feature or the handling itself..

The errormessage literally tells you, if the parameter was falsely valued, what its value should be and what it was. So its quite easy to actually determine where the error came from. If the handling itself is causing the error, the message will be self contradictory like:, "expecting, bool, but False was passed" - ... so i not really understand what complexity you refer to: the checks just return empty strings if check goes well and non-empty if they dont. At the end ValueError is thrown if any returned string is not empty (with the string being the suitable errormessage.) find that quite simple and neat.

I kind of sense it might be a thing you two: @schaefed and @palmb want to sort out first and overcome how you want to have it and which level of simplicity and implied redundance you want to go with.

Also the "devil lies in the details" a little bit. Because, if you might want to go for an more dead-simple approach, with plain conditions, like:

if condition(para_1) 
   raise ValueError(string)

keep in mind you will run into complexity problems also, as soon as you have parameters that can be different data-types (like window often being allowed to be a string or an integer), on wich condition might not always be applicable. (like if cond was range check using <)... so i think also a dead simple if-block approach might not be that straight forward at the end.

Without a thorough look into the changes (it is still a draft), I am very much in favor of the idea to generalize error checking and especially error messages. How this should be implemented and exact wording is, as always (also) a subjective thing.

A few responses from my side:

Error handling should be dead simple

The simplest approach would be to skip parameter value checking at all. The next simplest implementation would be largish if-else blocks in every function, which will end up in a lot of (IMO ugly) duplicated code. A bit more complicated in terms of code is the solution to provide functions that implement the relevant parameter checks. I, however, value consistency here (!) more the 'complexity' but also wouldn't consider calling a bunch of checking functions complex.

IMO the wording is quite difficult to read. in general its easier to understand positive names like isOffsetStr than noOffsetStr

Agree!

Error messages should be generated where the error occur, because it helps to understand the code

Don't agree! The error messages are for users. And I, as a user, strongly value consistency in error messages. Typos, different formulations for the same basic errors, differences in punctuation or capitalizations are for me no only a sign of disrespect of users, but also make the messages themselves harder to digest. This is all fine, as long as a project is in its early stages, but we aren't anymore, so we need to start to take care of such things. I.e. I like the idea to generate error messages, as I liked (and still like) the idea to generate docstrings for ubiquitous parameters like field, target and so on.

mentioned in merge request !691 (merged)

mentioned in merge request !675 (merged)

requested review from @palmb

mentioned in merge request !710 (merged)

closed