(original) (raw)

Nice working staying on top of this! Keeping up with discussion is
arguably much harder than actually writing the PEP. :) I have some
comments in-line below.

\-eric


On Fri, Sep 1, 2017 at 5:02 PM, Yury Selivanov <yselivanov.ml@gmail.com> wrote:
\> \[snip\]
\>
\> Abstract
\> ========
\>
\> \[snip\]
\>
\> Rationale
\> =========
\>
\> \[snip\]
\>
\> Goals
\> =====
\>

I still think that the Abstract, Rationale, and Goals sections should
be clear that a major component of this proposal is lookup via chained
contexts. Without such clarity it may not be apparent that chained
lookup is not strictly necessary to achieve the stated goals (i.e. an
async-compatible TLS replacement). This matters because the chaining
introduces extra non-trivial complexity.

> \[snip\]

On the one hand the first three sections imply that the PEP is
intended as a replacement for the current TLS mechanism;
threading.local. On the other hand, the PEP (and related discussion)
clearly says that the feature works differently than threading.local
and hence is not a (drop-in) replacement. I'd prefer it to be a
drop-in replacement but recognize I'm on the losing side of that
argument. :P Regardless, having a consistent message in the PEP would
help folks looking to switch over.

Speaking of which, I have plans for the near-to-middle future that
involve making use of the PEP 550 functionality in a way that is quite
similar to decimal. However, it sounds like the implementation of
such (namespace) contexts under PEP 550 is much more complex than it
is with threading.local (where subclassing made it easy). It would be
helpful to have some direction in the PEP on how to port to PEP 550
from threading.local. It would be even better if the PEP included the
addition of a contextlib.Context or contextvars.Context class (or
NamespaceContext or ContextNamespace or ...). :) However, I recognize
that may be out of scope for this PEP.