IBM / sqlalchemy-ibmi

Db2 for i SQL Alchemy adapter
Apache License 2.0
30 stars 12 forks source link

Improve _dbapi_version/_parse_dbapi_version and _get_driver_version #128

Open kadler opened 3 years ago

kadler commented 3 years ago

We imported the dbapi functions from the PyODBC dialect but these could be memoized like https://github.com/sqlalchemy/sqlalchemy/blob/fb81f9c8d914f9911925dd3f4e77d7fc374b267c/lib/sqlalchemy/dialects/postgresql/pg8000.py#L397

AFAICT, these functions are not actually used since we more care about the driver version than the PyODBC version, though it's possible that we may need to care if there's a bug that needs to be worked around in PyODBC. Since we don't inherit from PyODBC dialect, it's probably better to rename these to something more specific like _pyodbc_version and we could inline the _parse_dbapi_version.

In addition, the _get_driver_version could be replaced with a memoized property as well, eg. _driver_version.

markdirish commented 2 years ago

It seems that _dbapi_version and _parse_dbapi_version were copied from the pyodbc Connector logic (which I think is what you were hinting at above):

https://github.com/sqlalchemy/sqlalchemy/blob/f19e50ab75cfc904acef31434c92542f3ab50d61/lib/sqlalchemy/connectors/pyodbc.py#L158-L172

I agree that combining the two functions into one makes sense.

I also think that _get_driver_version and _get_server_version_info could both be memoized.

Should we use the sqlalchemy method of doing it (@util.memoized_property) or use the Python way, using @functools.lru_cache?

markdirish commented 2 years ago

I guess the question then becomes should we use memoized_instancemethod in the case where additional parameters are needed?

def _get_server_version_info(self, connection, allow_chars=True)

def _get_driver_version(self, db_conn):

Or possibly just create an ad hoc connection and make it a memoized_property? Not sure if that is something that's even possible, or if an instance of a class doesn't store connection string information to make such a thing.