-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
updated dbt.exceptions reference to exceptions in .sql files #1609
Conversation
Thanks @aminamos - this looks great! I think there's one tiny change to make to the Python code too. dbt "exports" specific methods from I think the last thing to do in this PR is to add
Will result in:
But after exporting the
I think once that's fixed, this PR will be good to merge! |
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.
Looking good @aminamos! I just kicked off the tests - we'll get back to you tomorrow on that CODE/MESSAGE question.
Thanks for making this PR!
core/dbt/config/profile.py
Outdated
@@ -148,7 +148,7 @@ def __eq__(self, other): | |||
if not (isinstance(other, self.__class__) and | |||
isinstance(self, other.__class__)): | |||
return False | |||
return False | |||
return False # 'unreachable code', remove line? |
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.
Yeah! Looks like you can remove this
core/dbt/exceptions.py
Outdated
@@ -236,7 +236,8 @@ class VersionsNotCompatibleException(SemverException): | |||
|
|||
|
|||
class NotImplementedException(Exception): | |||
pass | |||
CODE = 10010 |
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.
Hmm, not sure if we need / want to set these. I believe these CODE/MESSAGE attributes are only used in the rpc
server, and I'm not exactly sure how we decide which types of exceptions get their own codes.
@beckjake what do you think?
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.
Yeah, I'd just leave them at the defaults as inherited from Exception
. The decision process there is pretty ad-hoc: I ask myself "would it be valuable to know I got this error based on the error code if I were writing an RPC client?", and I think the answer here is "no", because I don't think we should ever get this error from the RPC server.
Apologies for how long that took, code has now been removed. Thank you! |
Thanks @aminamos - no worries at all - we appreciate you taking the time to contribute this fix :) Just kicked off the tests here - I think we'll be good to merge them when the tests pass! |
Co-Authored-By: Drew Banin <drew@fishtownanalytics.com>
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.
Thanks @aminamos - this is great! Merging -- this will go out in the 0.14.1 release
Creating draft PR to resolve #1569 (need to confirm whether changes should be made to dbt.exceptions module import in python files)