Hi Victor,
On Tue, Dec 12, 2017 at 6:49 PM, Victor Stinner
<victor.stinner at gmail.com> wrote:
> Hi Yury,
>> I like the overall idea and I prefer this PEP over PEP 550 since it's
> shorter and easier to read :-)
>> Question: Is there an API to list all context variables?
Context implements abc.Mapping, so 'get_context().keys()' will give
you a list of all ContextVars in the current context.
>> Would it be possible to have a very summary of the changes in the PEP?
> I propose:
>> """
> * Added contextvars module with ContextVar, Context and Token classes,
> and a get_context() function
> * asyncio: Added keyword-only context parameter to call_at(),
> call_later(), call_soon() methods of event loops and
> Future.add_done_callback(); Task are modified internally to maintain
> their own isolated context.
> """
Added.
>> Each get_context() call returns a new Context object. It may be worth
> to mention it. I understand why, but it's surprising that "assert
> get_context() is not get_context()" fails. Maybe it's a naming issue?
> Maybe rename it to contextvars.context()?
I think the name is fine. While get_context() will return a new instance
every time you call it, those instances will have the same context
variables/values in them, so I don't think it's a problem.
>>>> Abstract: ... This concept is similar to thread-local variables but, unlike TLS, ...
>> nitpick: please write "Thread Local Storage (TLS)". When I read TLS, I
> understand HTTPS (Transport Layer Security) :-)
Fixed.
[..]
>> ``contextvars.Token`` is an opaque object that should be used to
>> restore the ``ContextVar`` to its previous value, or remove it from
>> the context if it was not set before. The ``ContextVar.reset(Token)``
>> is used for that::
>>>> old = var.set(1)
>> try:
>> ...
>> finally:
>> var.reset(old)
>> I don't see where is the token in this example. Does set() return a
> token object? Yes according to ContextVar pseudo-code below.
>> When I read "old", I understand that set() returns the old value, not
> an opaque token. Maybe rename "old" to "token"?
Fixed.
>>>> The ``Token`` API exists to make the current proposal forward
>> compatible with :pep:`550`, in case there is demand to support
>> context variables in generators and asynchronous generators in the
>> future.
>> Cool. I like the idea of starting with something simple in Python 3.7.
> Then extend it in Python 3.8 or later (support generators), if it
> becomes popular, once the first simple (but "incomplete", without
> generators) implementation is battle-tested.
>>>> Any changes to any context variables that ``function`` causes, will
>> be contained in the ``ctx`` context::
>>>> var = ContextVar('var')
>> var.set('spam')
>>>> def function():
>> assert var.get() == 'spam'
>>>> var.set('ham')
>> assert var.get() == 'ham'
>>>> ctx = get_context()
>> ctx.run(function)
>>>> assert var.get('spam')
>> Should I read assert var.get() == 'spam' here?
Yes, fixed.
>> At the first read, I understood that that ctx.run() creates a new
> temporary context which is removed once ctx.run() returns.
>> Now I understand that context variable values are restored to their
> previous values once run() completes. Am I right?
ctx.run(func) runs 'func' in the 'ctx' context. Any changes to
ContextVars that func makes will stay isolated to the 'ctx' context.
>> Maybe add a short comment to explain that?
Added.
>> # Call function() in the context ctx
> # and then restores context variables of ctx to their previous values
> ctx.run(function)
>>>> Backwards Compatibility
>> =======================
>>>> This proposal preserves 100% backwards compatibility.
>> Ok.
>>> Libraries that use ``threading.local()`` to store context-related
>> values, currently work correctly only for synchronous code. Switching
>> them to use the proposed API will keep their behavior for synchronous
>> code unmodified, but will automatically enable support for
>> asynchronous code.
>> I'm confused by this sentence. I suggest to remove it :-)
>> Converting code to contextvars makes it immediately backward
> incompatible, no I'm not sure that it's a good it to suggest it in
> this section.
If we update decimal to use ContextVars internally, decimal will stay
100% backwards compatible.
Yury
