-
Notifications
You must be signed in to change notification settings - Fork 106
Reclassify user related errors #1760
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
Conversation
3a68557
to
cd7bf59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please see comments. Also, I was thinking of adding a base exception class like:
metricflow-semantics/metricflow_semantics/errors/error_classes.py
Outdated
Show resolved
Hide resolved
@@ -4,7 +4,13 @@ | |||
from typing import Dict, Optional | |||
|
|||
|
|||
class CustomerFacingSemanticException(Exception): | |||
class InformativeError(Exception): | |||
"""Raised when the error is actionable by the user or is informational.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What errors aren't informational
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's probably a better naming for this, but just trying to categorize it into the buckets of system/internal errors vs user/featurenotsupported/configerror/etc... and the latter I considered as informational to the user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to InformativeUserError
if that's any better and updated the docstring
@plypaul yea the |
To clarify, I haven't thought much about the exception class hierarchy, and the linked file was for reference, not that we need to put that in right now. |
Context
We throw a bunch of different exceptions for different scenarios. In attempts to categorize the errors appropriately, we will introduce a
InformativeError
which denotes that it isn't a system error from MF, but an error that falls under the bucket of things that a user can tweak in their input (either in their manifest config or query formulation) or it's just not supported.This isn't a complete solution, but there will be further efforts to classify more and make all the error handling be more verbose.