-
Notifications
You must be signed in to change notification settings - Fork 92
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
Improve TrinoStatementExecError
#4842
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4842 +/- ##
=====================================
Coverage 94.0% 94.0%
=====================================
Files 364 365 +1
Lines 30214 30288 +74
Branches 3599 3603 +4
=====================================
+ Hits 28389 28465 +76
+ Misses 1162 1159 -3
- Partials 663 664 +1 |
- Require parameters that are useful when handling this exception type - Expose the underlying TrinoQueryError directly - Use the __str__ method for formatting the error code - Use implicit string concatenation
- Don’t import the whole module - Correct repr - Increase coverage of trino_database.py
8ef5e7f
to
2681795
Compare
@@ -105,6 +134,44 @@ def cursor(self): | |||
select x from y; | |||
select a from b; | |||
""" | |||
with self.assertRaises(trino_db.TrinoStatementExecError): | |||
with self.assertRaises(ValueError): |
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.
nitpick question: Curious why this is now a ValueError
from TrinoStatementExecError
?
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.
I was hoping someone would ask about that. I changed the behavior in executescript()
to only raise TrinoStatementExecError
if a TrinoQueryError
was raised since that is now a required parameter of TrinoStatementExecError
. Otherwise, just raise the original exception.
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 for the explanation 👍🏾 , and I see the ValueError
raised by FakerFakeTrinoConn
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 👍🏾
|
Description
Improve the
TrinoStatementExecError
class to behave more like the exceptions fromtrino.exceptions
.Use the
__str__
method for message formatting.Make attributes of the underlying
TrinoQueryError
available as properties ofTrinoStatementExecError
. This allows for more precise error handling based on the specific type of Trino error rather than having to match on strings in the error message.Rename some one character variables.
Testing
tox -e py39 -- koku.test_trino_db_utils