zopefoundation / transaction

Generic transaction implementation for Python.
Other
70 stars 31 forks source link

Make before commit hooks raise exceptions again, broken in 2.4.1 #95

Closed sjustas closed 4 years ago

sjustas commented 4 years ago

Before commit hooks do not ask for cleaning and are expected to raise the exception.

Refactoring in 5aef8c2a accidentally introduced a return in a finally: block. Because a return in finally block takes priority over raised exception, if one hook failed, remaining hooks were simply not executed, no error was logged, and we proceeded with committing the transaction anyway.

sjustas commented 4 years ago

Done, let me know if I need to tweak the wording.

Thanks for the insanely fast response!

d-maurer commented 4 years ago

Justas Sadzevičius wrote at 2020-10-30 01:20 -0700:

Done, let me know if I need to tweak the wording.

There is no actual reraising. I suggest: "An exceptions raised during a before commit hook is no longer hidden but normally processed. Especially, no further commit hooks are called and the exception is propagated to the commit caller."

There is special (reStructuredText) syntax for links: text <url> In your case #95 <https://github.com/zopefoundation/transaction/pull/95>

Thanks for the insanely fast response! I was me who introduced the bug.

mgedmin commented 4 years ago

CI failures seem to be unrelated and should be fixed by #96.

It may be a good idea to merge that first and rebase this branch.

icemac commented 4 years ago

Thank you for your contribution.

According to the contributing policies of the zopefoundation organization you need to sign a contributor agreement before any non-trivial change before it can be merged. For details please consult the Contributing guidelines for zopefoundation projects.

icemac commented 4 years ago

96 is merged, please rebase your PR.

sjustas commented 4 years ago

According to the contributing policies of the zopefoundation organization you need to sign a contributor agreement before any non-trivial change before it can be merged. For details please consult the Contributing guidelines for zopefoundation projects.

Signed it again and sent to Plone Foundation. Previous one I signed (back in 2009) was pre-github I guess.

icemac commented 4 years ago

Sorry, my PR linting the package introduced conflicts here. Could you please rebase to the current master.

sjustas commented 4 years ago

@icemac no worries, glad to see a lint env!

icemac commented 4 years ago

@sjustas Do you need a new release of this package containing your fix?

strichter commented 3 years ago

@icemac A release with this fix would be great!

icemac commented 3 years ago

@mgedmin @jamadden I discovered that I do not have the rights to publish on PyPI. Could one of you please do the release or add me (icemac) to the list of allowed persons?

jamadden commented 3 years ago

Both done :)