clickhouse-sqlalchemy icon indicating copy to clipboard operation
clickhouse-sqlalchemy copied to clipboard

Fix dbapi error to comply with pep

Open Simon-Chenzw opened this issue 1 year ago • 6 comments

  • fixes #319

Checklist:

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

Simon-Chenzw avatar Jul 04 '24 04:07 Simon-Chenzw

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.

Simon-Chenzw avatar Jul 04 '24 04:07 Simon-Chenzw

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 avatar Aug 02 '24 06:08 Simon-Chenzw

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

Simon-Chenzw avatar Aug 02 '24 06:08 Simon-Chenzw

It's better to write

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

Yes, please write a test for this fix.

xzkostyan avatar Aug 06 '24 09:08 xzkostyan

Coverage Status

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

coveralls avatar Aug 28 '24 07:08 coveralls

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

Simon-Chenzw avatar Aug 28 '24 11:08 Simon-Chenzw