(original) (raw)

Some more feedback:

> This proposal builds directly upon concepts originally introduced
> in :pep:\`550\`.

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\`." ?

> The notion of "current value" deserves special consideration:
> different asynchronous tasks that exist and execute concurrently
> may have different values. This idea is well-known from thread-local
> storage but in this case the locality of the value is not always
> necessarily to a thread. Instead, there is the notion of the
> "current \`\`Context\`\`" which is stored in thread-local storage, and
> is accessed via \`\`contextvars.get\_context()\`\` function.
> Manipulation of the current \`\`Context\`\` is the responsibility of the
> task framework, e.g. asyncio.

This begs two (related) questions:

- If it's stored in TLS, why isn't it equivalent to TLS?

- If it's read-only (as mentioned in the next paragraph) how can the framework modify it?

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.

> Specification
> =============

> \[points 1, 2, 3\]

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.)

> The \`\`ContextVar\`\` class has the following constructor signature:
> \`\`ContextVar(name, \*, default=no\_default)\`\`.

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.

> class Task:
> def \_\_init\_\_(self, coro):

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.)

> CPython C API
> -------------
> TBD

Yeah, what about it? :-)

> The internal immutable dictionary for \`\`Context\`\` is implemented
> using Hash Array Mapped Tries (HAMT). They allow for O(log N) \`\`set\`\`
> operation, and for O(1) \`\`get\_context()\`\` function. \[...\]

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.)

> class \_ContextData

Since this isn't a real class anyway I think the \_\_mapping attribute might as well be named \_mapping. Ditto for other \_\_variables later.

--Guido van Rossum (python.org/\~guido)