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

update dbapi() to import_dbapi() #15

Merged
merged 3 commits into from
Mar 15, 2023

Conversation

david-engelmann
Copy link
Contributor

Latest Version of Turbodbc is raising this warning.

SADeprecationWarning: The dbapi() classmethod on dialect classes has been renamed to import_dbapi(). Implement an import_dbapi() classmethod directly on class <class 'sqlalchemy_turbodbc.dialect.MSDialect_turbodbc'> to remove this warning; the old .dbapi() classmethod may be maintained for backwards compatibility.

I've made that change and tested on this branch. The warning mentions that def dbapi(cls): can be kept as well if desired. I haven't in this branch but I'm happy to add it in before merging!

@dirkjonker
Copy link
Owner

the old .dbapi() classmethod may be maintained for backwards compatibility

I think this might be a good idea, wouldn't want to break things for people that happen to run old versions of things.

@david-engelmann
Copy link
Contributor Author

the old .dbapi() classmethod may be maintained for backwards compatibility

I think this might be a good idea, wouldn't want to break things for people that happen to run old versions of things.

I've added that in!

@dirkjonker
Copy link
Owner

Thank you!

@adamchen could you test this as well? And would you like to release it? Let me know if you need any help or if you want me to make a new release.

@adamchen
Copy link
Collaborator

adamchen commented Mar 6, 2023

Thanks for adding this @david-engelmann, and thanks for taking a look @dirkjonker! I'm away this week without my laptop but happy to take a look when I'm back this weekend.

@adamchen
Copy link
Collaborator

adamchen commented Mar 10, 2023

@david-engelmann, which version of Python are you using to test? I'm using 3.8.2 and still seeing the deprecation warning. I think it's down to the following snippet in sqlalchemy.engine.create:

if "import_dbapi" in dialect_cls.__dict__:
     dbapi_meth = dialect_cls.import_dbapi
elif hasattr(dialect_cls, "dbapi") and inspect.ismethod(dialect_cls.dbapi):
    util.warn_deprecated(....)
    dbapi_meth = dialect_cls.dbapi

It seems that import_dbapi is not a member of the __dict__ object, due to it being part of the TurbodbcConnector superclass. It seems that things work currently with the dbapi method because the engine creation code specifically checks that the attribute exists on the class and is a method, which it is.

To me, this signifies we should probably be putting both the dbapi and import_dbapi methods directly on the MSDialect_turbodbc class rather than in a subclass given SQLAlchemy's method for checking.

Let me know what you think, or if I've missed something

@david-engelmann
Copy link
Contributor Author

@adamchen Great observation, I've moved the logic into the MSDialect_turbodbc and I'm seeing the warning removed for multiple python versions. I have left the original dbapi class method in connector.py but can remove it entirely, if we'd like

@dirkjonker
Copy link
Owner

I think the dbapi method might be needed for compatibility with SQLAlchemy 1.x.? In that case it might be better to just keep it there so we don't accidentally break anything for anyone.

@david-engelmann
Copy link
Contributor Author

I think the dbapi method might be needed for compatibility with SQLAlchemy 1.x.? In that case it might be better to just keep it there so we don't accidentally break anything for anyone.

I figured, I'm definitely down to leave the dbapi method for backwards compatibility

@adamchen
Copy link
Collaborator

Good stuff, thanks @david-engelmann and appreciate the guidance @dirkjonker! I'll merge this and get a release built Weds.

@adamchen adamchen merged commit e80212b into dirkjonker:master Mar 15, 2023
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.

3 participants