search

zopefoundation / zope.sqlalchemy

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

Support transaction>=1.6.0 #25

Closed drnextgis closed 6 years ago

drnextgis commented 6 years ago

Starting from transaction 1.6.0 (see https://github.com/zopefoundation/transaction/commit/2eb00f903002cd170ea66c89856c95b6df4e8066 commit) references to data managers are dropped when transaction committed or aborted.

tseaver commented 6 years ago

Given that we are making this change only for testing purposes, I think I would be inclined to remove the pin and make the tests more complex (e.g., with a version test or similar).

drnextgis commented 6 years ago

@tseaver thanks for help. I've updated PR.

tseaver commented 6 years ago

LGTM. @icemac does it suit you?

icemac commented 6 years ago

Sorry, but I do not share the view of @tseaver: transaction 1.6 was released at 2016-05-21 aka about 1,5 years ago. It is 1 major and 3 minor releases behind the current release of 2.1.2. I see no need to have special tests for such an ancient version. If someone still has to use the old transaction version, he can still use the current release of zope.sqlalchemy. We could even release a new major version saying: dropped support for transaction < 1.6.

@drnextgis I am not sure if the test is still meaningful: In transaction < 1.5 it was possible to check that we are joined to the transaction even after calling transaction.abort(). transaction >= 1.6 changed the behaviour (as you have written in the PR description), so we now are no longer joined to the transaction after calling transaction.abort(). Checking that the list of resources is empty means only checking that transaction >= 1.6 is used. Maybe the two asserts can be removed. At least the message "Not joined transaction" is now wrong: if the assertion fails we are still joined to the transaction.

drnextgis commented 6 years ago

It makes sense. I've removed an obsolete asserts.

icemac commented 6 years ago

@tseaver Are the changes okay for you?

drnextgis commented 6 years ago

@icemac could you merge this PR?