Skip to content

Conversation

bluetech
Copy link
Member

[Resubmission of #6789 -- see previous discussion there.]

pytest has several instances where plugins set their own attributes on
objects they receive in hooks, like nodes and config. Since plugins are
detached from these object's definition by design, this causes a problem
for type checking because these attributes are not defined and mypy
complains.

Fix this by giving these objects a "store" which can be used by plugins
in a type-safe manner.

Currently this mechanism is private. We can consider exposing it at a
later point.

…jects

pytest has several instances where plugins set their own attributes on
objects they receive in hooks, like nodes and config. Since plugins are
detached from these object's definition by design, this causes a problem
for type checking because these attributes are not defined and mypy
complains.

Fix this by giving these objects a "store" which can be used by plugins
in a type-safe manner.

Currently this mechanism is private. We can consider exposing it at a
later point.
@nicoddemus
Copy link
Member

nicoddemus commented Feb 29, 2020

This is great work @bluetech, thanks!

To give some context, @RonnyPfannschmidt and I have discussed in the past of having a "store" mechanism for Nodes, similar to what you have here, but with a different intention: to allow plugins to store scoped data in items. For example, you could store an object in a Class node and have it automatically cleaned up when the Class is torn down. Besides useful in general, the more ambitious intent is to use this as the store mechanism for fixtures. Currently the fixture cache mechanism is done in the FixtureDef class, and is quite complicated, so we hope this "scoped store" mechanism would greatly simplify things.

I have thought that this mechanism would also have a "config/global" scope, which would store items in config, and torn down automatically when config gets destroyed, which is exactly what you have here. 😁

This is just to give you context, I think the PR is already a huge improvement and a step on the right direction, so I wouldn't like to burden it further with changes.


As a next step after this is merged, we could discuss how one could store an object and also a way to customize the cleanup of that object when the store gets out of scope/is destroyed.

For example (from this PR):

fault_handler_stderr_key = StoreKey[TextIO]()

def pytest_configure(config):
    ...
    config._store[fault_handler_stderr_key] = open(stderr_fd_copy, "w")

def pytest_unconfigure(config):
    ...
    config._store[fault_handler_stderr_key].close()
    del config._store[fault_handler_stderr_key]

It would be neat if we can somehow define a finalizer and avoid having to cleanup manually, for example (just an idea):

fault_handler_stderr_key = StoreKey[TextIO](finalizer=lambda x: x.close())

def pytest_configure(config):
    ...
    config._store[fault_handler_stderr_key] = open(stderr_fd_copy, "w")

So every object stored by fault_handler_stderr_key will have its finalizer called automatically when the store gets destroyed.

This would of course be useful as next step, and also a further step on the idea of having stores on items. 👍

@RonnyPfannschmidt
Copy link
Member

Let's avoid to accidentially integrate life cycles with the storage, after we have a base we can think of a layer on top to express creation, destruction and dependencies

@bluetech
Copy link
Member Author

Thanks for the explanation @nicoddemus. It'd be interesting to see how it works out. And since the type is private, and can be adjusted as needed.

So I think the only thing left is @RonnyPfannschmidt request to make things "extra private" by adding prefix _StoreKey, _Store. I'd personally prefer to change the module store.py -> _store.py and keep the class names unprefixed, just because it's less ugly :) But I don't mind either way. WDYT @RonnyPfannschmidt?

@RonnyPfannschmidt
Copy link
Member

as we are actually under _pytest, i believe its fine to leave things as they are right now

@nicoddemus nicoddemus merged commit 92767fe into pytest-dev:master Mar 1, 2020
@bluetech bluetech deleted the store branch March 1, 2020 19:02
@blueyed blueyed changed the title Add a typing-compatible mechanism for ad-hoc attributes on various objects Add a typing-compatible mechanism for ad-hoc attributes on various objects (Store) Mar 2, 2020
store[key1] = "world"
assert store[key1] == "world"
# Has correct type (no mypy error).
store[key1] + "string"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bluetech
I've wondered if this could be improved?

    # > Cannot infer type argument 1 of "__setitem__" of "Store"
    store[key1] = 42

E.g. also by a runtime check, and/or better message?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the message can be improved. Sounds like something mypy itself can improve.

Regarding runtime checks, why do you think they should be added?

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.

4 participants