Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add __slots__ to AsyncResource #733

Merged
merged 3 commits into from May 12, 2024
Merged

Add __slots__ to AsyncResource #733

merged 3 commits into from May 12, 2024

Conversation

injust
Copy link
Contributor

@injust injust commented May 12, 2024

Changes

AsyncResource should have __slots__ so child classes can use __slots__.

Similar to python/cpython#105726 and python/cpython#106771 for contextlib.AbstractAsyncContextManager.

Checklist

If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):

  • You've added tests (in tests/) added which would fail without your patch
  • You've updated the documentation (in docs/, in case of behavior changes or new
    features)
  • You've added a new changelog entry (in docs/versionhistory.rst).

@agronholm
Copy link
Owner

Do you have a use case for this? None of the subclasses in AnyIO itself use __slots__.

@injust
Copy link
Contributor Author

injust commented May 12, 2024

I'm creating my own child classes of AsyncResource with attrs, and was surprised that they weren't slotted classes. I'm assuming that's a supported use case, similar to how one might use contextlib.AbstractAsyncContextManager.

@agronholm
Copy link
Owner

Attrs doesn't require the superclasses to have __slots__ in order to create a slotted class:

>>> import attrs
>>> class Foo:
...   pass
... 
>>> @attrs.define(slots=True)
... class Bar:
...   x: int
... 
>>> b = Bar(1)
>>> b.y = 5
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'Bar' object has no attribute 'y'

@injust
Copy link
Contributor Author

injust commented May 12, 2024

You forgot to subclass Bar from Foo in your example.

From https://threeofwands.com/attrs-ii-slots/ (which is a really good write-up on slotted classes btw):

Slot classes lose a bunch of their advantages if the inheritance chain contains even a single dict class, because there will be an instance dictionary associated with every instance. Slot classes all the way!

@agronholm
Copy link
Owner

Right, I forgot to do that. It seems that attrs has changed the way it operates, because it used to work this way (although I will say it was the wrong thing to do). The only question that remains then, is whether we want to include the __weakref__ slot here.

@injust
Copy link
Contributor Author

injust commented May 12, 2024

It seems that attrs has changed the way it operates, because it used to work this way

I haven't been using attrs for too long so I can't comment on that. Only thing that comes to mind is that slotted classes are default for attrs.define and attrs.frozen,

whether we want to include the __weakref__ slot here

I'm -0 on this. If someone wanted a weak referenceable-AsyncResource, they could create their own subclass (and have it be slotted or not).

@agronholm agronholm merged commit dfc44cf into agronholm:master May 12, 2024
16 checks passed
@agronholm
Copy link
Owner

Alright, you convinced me.

@injust injust deleted the patch-1 branch May 12, 2024 22:16
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.

None yet

2 participants