Skip to content

Conversation

@dokime7
Copy link

@dokime7 dokime7 commented Jul 5, 2021

In order to use 'transaction' on an asyncio framework like FastAPI, it's needed to use ContextVar instead of threading.local that is not compatible with asyncio.

ContextVar is fully compatible with threading.local and can be used as a replacement of.

ContextVar is a Python standard lib since 3.7 and can be installed as a lib on 3.6 but unfortunately not compatible with older Python versions.

@jugmac00
Copy link
Member

jugmac00 commented Jul 5, 2021

@dokime7 Thanks for the PR - please note, that this package needs to stay compatible with Python 2.7 and Python 3.5

@dokime7
Copy link
Author

dokime7 commented Jul 5, 2021

OK, I will update my PR

@dokime7
Copy link
Author

dokime7 commented Jul 5, 2021

Updated, feel free to give feedback.

@icemac
Copy link
Member

icemac commented Jul 6, 2021

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 can be merged. For details please consult the Contributing guidelines for zopefoundation projects.

@icemac
Copy link
Member

icemac commented Jul 6, 2021

This package uses https://pypi.org/project/coverage-python-version/ so marking the code paths needed for Python 2 resp. 3 should bring the coverage back to the expected value.

@icemac icemac requested review from jamadden and mgedmin July 6, 2021 06:28
@dokime7
Copy link
Author

dokime7 commented Jul 6, 2021

OK, I've pushed a fix

icemac
icemac previously requested changes Jul 6, 2021
Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I meant with my comment regarding https://pypi.org/project/coverage-python-version/.

Explanation: We are running coverage only on Python 3.8 so code needed just for older Python versions (especially) Python 2.7 should be marked with # pragma: PY2 instead of no cover as we want to see coverage for code which has different branches for different Python versions.

dokime7 and others added 2 commits July 6, 2021 14:13
@dokime7
Copy link
Author

dokime7 commented Jul 6, 2021

OK, sorry, it is much clear for me now :)

@icemac icemac dismissed their stale review July 7, 2021 06:21

Requested changes where applied.

@dataflake
Copy link
Member

This still requires signing the contributor agreement. Please see the Contributing guidelines for zopefoundation projects.

class ThreadTransactionManager(threading.local):
"""Thread-local
`transaction manager <transaction.interfaces.ITransactionManager>`.
if USE_CONTEXTVAR: # pragma: PY3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you switch the if order it might make the diff much more easy to read:

if not USE_CONTEXTVAR:
   # old/current code that on the diff is only shown as being indented
else:
  # new code, that shows up as new code

otherwise a quick glimse at the code feels like the new code is actually the old one 😅

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.

5 participants