<div dir="ltr"><div><div><div><div><div><div><div><div><div><div>Some more feedback:<br><br>> This proposal builds directly upon concepts originally introduced<br>> in :pep:`550`.  <br><br></div>The phrase "builds upon" typically implies that the other resource must be read and understood first. I don't think that we should require PEP 550 for understanding of PEP 567. Maybe "This proposal is a simplified version of :pep:`550`." ?<br><br>> The notion of "current value" deserves special consideration:<br>> different asynchronous tasks that exist and execute concurrently<br>> may have different values.  This idea is well-known from thread-local<br>> storage but in this case the locality of the value is not always<br>> necessarily to a thread.  Instead, there is the notion of the<br>> "current ``Context``" which is stored in thread-local storage, and<br>> is accessed via ``contextvars.get_context()`` function.<br>> Manipulation of the current ``Context`` is the responsibility of the<br>> task framework, e.g. asyncio.<br><br></div>This begs two (related) questions:<br></div>- If it's stored in TLS, why isn't it equivalent to TLS?<br></div>- If it's read-only (as mentioned in the next paragraph) how can the framework modify it?<br><br></div>I realize the answers are clear, but at this point in the exposition you haven't given the reader enough information to answer them, so this paragraph may confuse readers.<br><br>> Specification<br>> =============<br></div>> [points 1, 2, 3]<br><br></div>Shouldn't this also list Token? (It must be a class defined here so users can declare the type of variables/arguments in their code representing these tokens.)</div><div><br></div><div>> The ``ContextVar`` class has the following constructor signature:<br>> ``ContextVar(name, *, default=no_default)``.</div><div><br></div><div>I think a word or two about the provenance of `no_default` would be good. (I think it's an internal singleton right?) Ditto for NO_DEFAULT in the C implementation sketch.<br></div><div><div><br></div><div>>     class Task:<br>>         def __init__(self, coro):<br></div><div><br></div><div>Do
 we need a keyword arg 'context=None' here too? (I'm not sure what would
 be the use case, but somehow it stands out in comparison to 
call_later() etc.)<br></div><br>> CPython C API<br>> -------------<br>> TBD<br><br></div>Yeah, what about it? :-)<br></div><div><br></div><div>> The internal immutable dictionary for ``Context`` is implemented<br>> using Hash Array Mapped Tries (HAMT).  They allow for O(log N) ``set``<br>> operation, and for O(1) ``get_context()`` function.  [...]</div><div><br></div><div>I wonder if we can keep the HAMT out of the discussion at this point. I have nothing against it, but given that you already say you're leaving out optimizations and nothing in the pseudo code given here depends on them I wonder if they shouldn't be mentioned later. (Also the appendix with the perf analysis is the one thing that I think we can safely leave out, just reference PEP 550 for this.)<br></div><div><br></div>> class _ContextData<br><br></div>Since this isn't a real class anyway I think the __mapping attribute might as well be named _mapping. Ditto for other __variables later.<br><div><div><div><div><div><div><div><div><div><br><div><div><div class="gmail_extra">-- <br><div class="gmail_signature">--Guido van Rossum (<a href="http://python.org/~guido" target="_blank">python.org/~guido</a>)</div>
</div></div></div></div></div></div></div></div></div></div></div></div></div>