fix: address __slots__ and __all__ misconfigurations
#3273
Conversation
There was a problem hiding this comment.
Thanks very much for identifying this issue @ariebovenberg, and for submitting the patch!
I think slotscheck is an important tool for us, and by the looks of this we had a couple of configuration issues (i.e., the exclude regex and improper environment setup). Is it possible for the tool to be run outside of pre-commit? We recently removed mypy and pyright from pre-commit due to having to maintain multiple lists of dependencies which would frequently be forgotten about as the library dependencies changed.
I have particular concern with flake8-dunder-all, firstly the way it injects the __all__ in between the runtime and typing imports, and secondly I feel that the names added to __all__ needs a bit more consideration than writing any non-underscore name in there, especially in our public namespaces.
@litestar-org/maintainers any thoughts on flake8-dunder-all?
| __all__ = ( | ||
| "AsyncDeque", | ||
| "Subscriber", | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Personally, not a fan that flake8-dunder-all injects this in between runtime imports and type-checking imports.
I'm also not sure whether blindly adding any non-underscore names into an __all__ is desirable. For instance in this case, I don't believe AsyncDeque is intended to be part of the modules public interface.
There was a problem hiding this comment.
i can think of a few instances we havent privatized a thing and want it undocumented, but im more tilted about how it doesnt place them under TYPE_CHECKING imports as well :D
| @@ -21,7 +21,7 @@ | |||
| class RedisStore(NamespacedStore): | |||
| """Redis based, thread and process safe asynchronous key/value store.""" | |||
|
|
|||
| __slots__ = ("_redis",) | |||
| __slots__ = ("_redis", "handle_client_shutdown", "_get_and_renew_script", "_delete_all_script") | |||
There was a problem hiding this comment.
probably a dumb question but why do we expose private methods here
There was a problem hiding this comment.
note that __slots__ isn't about exposing things like __all__ is. __slots__ merely tells Python to only allow certain attributes on the class in order to optimize memory. Private attributes are no exception.
There was a problem hiding this comment.
ahh yep.. i thought this was __all__ i think i got lost in the buzz.
| "warn_pdb_on_exception", | ||
| "warn_sync_to_thread_with_async_callable", | ||
| "warn_sync_to_thread_with_generator", | ||
| ) |
There was a problem hiding this comment.
idk if we want these exposed in documentation
There was a problem hiding this comment.
I decided to leave the flake8 dunder additions unmodified so those more experienced with the library can decide what to exclude 👍
|
I made a PR to see if we can get f8-d-a to play nice with the type check imports - don't really know what I'm doing though, so lets see what happens: python-formate/flake8-dunder-all#65 |
They released f8-d-a 0.4 which has support for an WRT changing how we use the tool, it might be better to use it as a lint without the auto-fix, so it forces us to have the |
Fixes #3251
Remarks:
__all__definitions, which I left unchanged.language: system, but this negates the benefit of pre-commit environment isolation.__slots__working onAbstractMiddlewarewithout making breaking changes. This is due to the fact that the slot names are used both as instance and class variables. To prevent confusion, I've removed the (ineffective) slots from its subclasses__slots__toAbstractContextManagerandAbstractAsyncContextManagerpython/cpython#106771) renders slots on event emitters ineffective. Since this will roll out in Python 3.13, I've kept the slots