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

Fix dbapi error to comply with pep #320

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Simon-Chenzw
Copy link
Contributor

@Simon-Chenzw Simon-Chenzw commented Jul 4, 2024

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Ensure PR doesn't contain untouched code reformatting: spaces, etc.
  • Run flake8 and fix issues.
  • Run pytest no tests failed. See https://clickhouse-sqlalchemy.readthedocs.io/en/latest/development.html.

@Simon-Chenzw
Copy link
Contributor Author

It changes the native and http and asynch.
I am a little confused about the native and http.
The native wraps the error in DatabaseException. Does it ensure that all errors are caught as DatabaseException? Can we delete DatabaseException?
For http, what is the basic error? Exception may be too wide.

You can use this to catch all errors with one single except statement.
"""
pass
Error = DatabaseException
Copy link
Owner

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.

Copy link
Contributor Author

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?

@Simon-Chenzw
Copy link
Contributor Author

Updated.
Will only change asynch dbapi error.
By the way,

class Error(Exception):
    pass

This does not mean "catch all exceptions". This creates a new exception called "Error", but no one will ever try to raise it.

@Simon-Chenzw
Copy link
Contributor Author

If you agree to this change, I will write a unittest ASAP.

@xzkostyan
Copy link
Owner

It's better to write

from asynch.errors import ClickHouseException
...
    Error = ClickHouseException

Yes, please write a test for this fix.

@coveralls
Copy link

coveralls commented Aug 28, 2024

Coverage Status

coverage: 86.276% (-0.005%) from 86.281%
when pulling 0a055c7 on Simon-Chenzw:master
into 523559e on xzkostyan:master.

@Simon-Chenzw
Copy link
Contributor Author

Test added, sorry for the wait.
I think it's a breaking change so we need to update the changelog?

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

Successfully merging this pull request may close these issues.

Sqlalchemy can't catch asynch's error
3 participants