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

Replace treading.local with contextvar #107

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

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.

src/transaction/_manager.py Outdated Show resolved Hide resolved
src/transaction/_manager.py Outdated Show resolved Hide resolved
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