Skip to content

Replace treading.local with contextvar - #107

Open
dokime7 wants to merge 5 commits into
zopefoundation:masterfrom
dokime7:contextvar
Open

Replace treading.local with contextvar#107
dokime7 wants to merge 5 commits into
zopefoundation:masterfrom
dokime7:contextvar

Conversation

@dokime7

@dokime7 dokime7 commented Jul 5, 2021

Copy link
Copy Markdown

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

jugmac00 commented Jul 5, 2021

Copy link
Copy Markdown
Member

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

@dokime7

dokime7 commented Jul 5, 2021

Copy link
Copy Markdown
Author

OK, I will update my PR

@dokime7

dokime7 commented Jul 5, 2021

Copy link
Copy Markdown
Author

Updated, feel free to give feedback.

@icemac

icemac commented Jul 6, 2021

Copy link
Copy Markdown
Member

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

icemac commented Jul 6, 2021

Copy link
Copy Markdown
Member

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

dokime7 commented Jul 6, 2021

Copy link
Copy Markdown
Author

OK, I've pushed a fix

icemac
icemac previously requested changes Jul 6, 2021

@icemac icemac left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/transaction/_manager.py Outdated
Comment thread src/transaction/_manager.py Outdated
dokime7 and others added 2 commits July 6, 2021 14:13
Co-authored-by: Michael Howitz <mh@gocept.com>
Co-authored-by: Michael Howitz <mh@gocept.com>
@dokime7

dokime7 commented Jul 6, 2021

Copy link
Copy Markdown
Author

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
Copy Markdown
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
Copy Markdown
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