search

crate / sqlalchemy-cratedb

SQLAlchemy dialect for CrateDB.
https://cratedb.com/docs/sqlalchemy-cratedb/
Apache License 2.0
3 stars 2 forks source link

SqlAlchemy Driver is missing the `.driver` attribute #120

Closed jvanasco closed 4 years ago

jvanasco commented 4 years ago

Please see https://github.com/sqlalchemy/sqlalchemy/issues/5322

also note the following comment from SqlAlchemy's primary author:

The dialect compliance test suite has many areas in which it assumes ".driver"is available so this would indicate the crate dialect is not running the compliance test suite.

autophagy commented 4 years ago

@jvanasco Thanks for bringing this to our attention. I'll look into fixing this, as well as making sure the sqlalchemy compliance suite is being run.

autophagy commented 4 years ago

@jvanasco Would you be able to verify that this has been fixed with 751c9018d6a91460bdfb583083146f73f800c0a3 ? This is now part of the 0.24.0 release of the Crate driver.

jvanasco commented 4 years ago

So this change generally LGTM. I checked with the core devs because I had some concerns over the '-' in the name, but everyone thinks it should be fine. No one believes it is being used as a Python identifier.

The one concern I have is that this isn't hooked-into the compliance tests yet. I suggest you take a quick look at the dialect testing guidelines at https://github.com/sqlalchemy/sqlalchemy/blob/master/README.dialects.rst

It shouldn't be more than a few hours of work to leverage the SqlAlchemy testing suite into this client, and that should help save a lot of headaches in the future.

autophagy commented 4 years ago

Thank you for the heads up. I think in the past we had some problems with integrating the compliance suite due because of a lack of transactions, but I think this was long before I came to the project. I'll open another issue to investigate & try integrating the compliance suite into the driver.

autophagy commented 4 years ago

I'll also close this issue for now. Thank you, @jvanasco !