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

Would it be possible to support __slots__? #31

Closed
Tinche opened this issue Nov 11, 2015 · 9 comments
Closed

Would it be possible to support __slots__? #31

Tinche opened this issue Nov 11, 2015 · 9 comments

Comments

@Tinche
Copy link
Member

Tinche commented Nov 11, 2015

I don't know if this would even be technically possible, but it'd still be nice to have. I'm personally not that interested in the efficiency gains (although it'd be nice), but I'd like for exceptions to pop up when non-existent attributes are assigned to attrs instances (just got bitten by this making one of my tests fail).

@hynek
Copy link
Member

hynek commented Nov 11, 2015

I’m afraid not trivially. Setting __slots__ on class attributes makes them constant…

I think there might be a way via descriptors but I hadn’t time to investigate any further.

@Tinche
Copy link
Member Author

Tinche commented Mar 3, 2016

I've been playing with maybe implementing this. I think I might have something; I'll have to parametrize the tests before I know whether it works.

I think the point of __slots__ is lost if you inherit from a class that doesn't use slots - as far as I can see there are no size gains (or they are much smaller) and arbitrary attributes can be assigned. I'm thinking throwing an error if we detect a non-slots class in the class hierarchy when creating?

So my working idea is to create a new class in wrap (https:/hynek/attrs/blob/master/src/attr/_make.py#L185) and adapt that. The problem here is __slots__ doesn't work with class variables.

>>> class A():
...     __slots__ = ("t")
...     t = "test"
...
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: 't' in __slots__ conflicts with class variable

This is technically an API break for slots classes. (The docs for attr.Attribute state: "Instances of this class are frequently used for introspection purposes like: [...] Class attributes on attrs-decorated classes after @attr.s has been applied.")

There may be other edge cases since we're basically replacing one class with another, but these should be very rare and could be documented.

So my initial idea was to just add a slotskwarg to attr.s(), but maybe slot classes could be a separate thing? They need additional documentation, can't support the entire interface and are most useful with only other slots classes in the inheritance hierarchy.

Would appreciate your thoughts :)

@hynek
Copy link
Member

hynek commented Mar 9, 2016

Hm yeah replacing classes sounds really shaky but AFAICT, it’s the only way. They’d have to be something opt-in anyway, so I don’t see a problem here.

In any case, it would be a new type, new API so we have total freedom as long as stuff is documented. Especially stuff like putting limits on slots only on top of slots etc. One can always loosen restriction but once the genie is out of the bottle…

One thing to consider is how inheritance works out?

@Tinche
Copy link
Member Author

Tinche commented Mar 14, 2016

I've got something brewing over here: https:/Tinche/attrs/tree/feature/slots

I've covered the simple stuff, adjusted some tests and added some new ones. Went with the following approach, since it seems natural and as you said slots are opt-in:

@attr.s(slots=True)
class MyClass(object):
    x = attr.ib()

It seems to work for simple cases.

Now to cover the more complex stuff, like ensuring more complex classes can be transformed into slotted ones correctly. I think I've changed my mind on putting explicit restrictions on inheriting from non-slots classes; this being Python (consenting adults) and the fact that it's possible in pure Python, I don't think we should ban it. It should just be well documented.

@hynek
Copy link
Member

hynek commented Mar 15, 2016

You’re not asking for feedback, are you? :) AFAICT you’ve implemented what we talked about so far.

@Tinche
Copy link
Member Author

Tinche commented Mar 15, 2016

It's still early, this is me exploring. Feedback is always welcome though :)

@Tinche
Copy link
Member Author

Tinche commented Mar 16, 2016

Alright, I've polished up the implementation a little and thrown a lot of tests (old and new) at it, and it seems to be working. Would you like me to make a pull request now?

Apart from reviewing the code, we can talk about how exactly would you like this documented.

@hynek
Copy link
Member

hynek commented Mar 17, 2016

Sure, PRs are free. :)

hynek added a commit that referenced this issue Mar 31, 2016
hynek added a commit that referenced this issue Mar 31, 2016
@hynek
Copy link
Member

hynek commented Mar 31, 2016

Fixed by you! \o/ :)

@hynek hynek closed this as completed Mar 31, 2016
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

No branches or pull requests

2 participants