-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
Fix dbapi error to comply with pep #320
base: master
Are you sure you want to change the base?
Conversation
It changes the native and http and asynch. |
You can use this to catch all errors with one single except statement. | ||
""" | ||
pass | ||
Error = DatabaseException |
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.
Why do we change Exception to DatabaseException? Ordinary exceptions can be thrown as well.
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'm just a little confused about what DatabaseException does.
If the ordinary exceptions is also thrown, then it should be Exception
or clickhouse_driver.Error
?
Updated.
This does not mean "catch all exceptions". This creates a new exception called "Error", but no one will ever try to raise it. |
If you agree to this change, I will write a unittest ASAP. |
It's better to write from asynch.errors import ClickHouseException
...
Error = ClickHouseException Yes, please write a test for this fix. |
1b89131
to
4804e76
Compare
Test added, sorry for the wait. |
Checklist:
flake8
and fix issues.pytest
no tests failed. See https://clickhouse-sqlalchemy.readthedocs.io/en/latest/development.html.