search

zopefoundation / zope.sqlalchemy

Integration of SQLAlchemy with transaction management
Other
32 stars 34 forks source link

zope.sqlalchemy is still using SessionExtension #31

Closed zzzeek closed 5 years ago

zzzeek commented 5 years ago

Hi there -

Figured I'd take a look at this to see what the story is. So, part of the blame is on me since apparently I haven't had SessionExtension / MapperExtension / etc. emit deprecation warnings, I'm going to see if I can get that in for 1.3. However the Extension classes have been documented as deprecated since 2012.

It looks like, and I think I probably helped with this, that you have built up the register system that uses the (not really new anymore) event API. So, that has to be where this goes, because SessionExtension as well as the extension parameter on Session will go away in a future relase, (I don't want to say what release, but there is a rumor that it may not include the digit "1" inside of it).

So I think the methods of ZopeTransactionExtension need to be separated off of SessionExtension so you can still use them in register, then the actual ZopeTransactionExtension can be isolated from it. Then you will want to update your README to document the register() version of things as how people should be doing it (which has been for six years now :) ). Again I hope to get some deprecation warnings in 1.3 to help this all along.

Blame is on me for not emitting deprecation warnings for the extension parameter as well as use of SessionExtension etc., I'm going to add that to my list of todos.

icemac commented 5 years ago

The deprecation warnings are landed in SQLAlchemy 1.3 so a volunteer is needed to update zope.sqlalchemy so it can run without the deprecated code.

icemac commented 5 years ago

Could the solution be as easy as dropping the parent class from ZopeTransactionExtension?

https://github.com/zopefoundation/zope.sqlalchemy/blob/67bc1274e32626c464c8a94c7c3ef16dc06102e6/src/zope/sqlalchemy/datamanager.py#L232

We already use event.listen. Maybe ZopeTransactionExtension should be renamed too, so users are pushed towards using the register function.

ctheune commented 5 years ago

Yeah, @icemac that was pretty much exactly the solution I ended up with ... :)