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

Discussion: Error Codes #80

Open
jranalli opened this issue Jan 19, 2023 · 2 comments
Open

Discussion: Error Codes #80

jranalli opened this issue Jan 19, 2023 · 2 comments

Comments

@jranalli
Copy link
Collaborator

As we're trying to write more front-end friendly code (e.g. PMGI), I'm finding that the error cases we have don't quite provide enough info. PMParamError can mean you specified P, T and d. It can also mean you specified an insane property called JX.

Proposed solutions: more error types or embed concrete error codes with the error so client code can avoid trying to process strings.

Happy to dig into either solution pending discussion.

@chmarti1
Copy link
Owner

I think the better approach is to provide mechanisms for the error messages to be passed to the front end. Seeing the error type is helpful, but a meaningful message is always better. The other advantage is that avoids needing to dig through ALL of the code to rewrite all error handling to be consistent with a new type.

@jranalli
Copy link
Collaborator Author

jranalli commented Jan 20, 2023

I don't think it would be possible to get the error messages to do all the lifting on this, because the users have different goals in mind. As an example, when too many parameters are specified, I'd like our educational front end code to be very elaborate in the explanation and maybe even link to a page about the state principle, while a CLI user would probably not like that maximum verbosity error message.

We could say "just parse the string to figure out which type it was", but that would require that the exact text of error messages is guaranteed immutable in future versions. There are also different exact forms of the text that are used by different PM classes and different parts of the code within those, so we'd need to clean them up.

This is what I really had in mind, because I believe it's simpler than trying to guarantee parsable messages:

class PMParamError(Error):
    def __init__(self, message, subcode=0):
        super().__init__(message)
        self.subcode = subcode

We could then agree on just a few codes for a few specific cases (e.g. state principle violations as compared to temperature-in-bounds violations). And any client code that cares can now just check for error.code, and any code that doesn't care doesn't need to change. Specifying a default/type_unspecified code would prevent us from having to change every single instance of PMParamError, but would still allow new specific codes to be added piecemeal if any other cases of these come up. And to be clear, I am volunteering to be the one who goes through the relevant parts of the codebase to add these codes, so it's not necessarily a big time suck for you. I think these are all going to come from the various substance class files.

A quick look through mp1.py and ig.py indicates that it could be as simple as 6 total cases for PMParamError to cover those whole files:

  • Positional arg errors (not really important for GUI driven code, could be general?)
  • State principle violation (specified more than 2)
  • Unsolvable case (specified two properties that can't be solved together)
  • Saturation bounds issues (above or below crit pt)
  • Model bounds issues (T or P is outside of the allowed range)
  • Implicit bounds issues (i.e. bracketing. Your chosen p & h puts T out of bounds)

Sidebar: the other way to do it piecemeal without breaking things would be if you wanted to actually make these new error types that inherit from PMParamError. Any code that tries to catch PMParamError would still catch those subtypes, but that seems ickier and more prone to unexpected results to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants