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

Rect is iterable but not typed or documented #2681

Closed
vnmabus opened this issue Aug 17, 2021 · 15 comments · Fixed by #2969 or #3066
Closed

Rect is iterable but not typed or documented #2681

vnmabus opened this issue Aug 17, 2021 · 15 comments · Fixed by #2969 or #3066
Labels
rect pygame.rect types Type hint checking related tasks

Comments

@vnmabus
Copy link
Contributor

vnmabus commented Aug 17, 2021

So, it seems that Rect is iterable and you can do

r = Rect(0, 1, 2, 3)

x, y, w, h = r

However I do not see this documented anywhere, nor exposed in the type hints. Thus, Mypy complains but the code works.

Is this an oversight, or is this feature not supposed to be used?

@Starbuck5
Copy link
Contributor

I knew about this, I don't remember how though.

Rect objects are just supposed to behave like sequences of 4 numbers, and iteration is part of that.

They can also be directly indexed, is getitem typed?

It would be great to have a small note in the docs about these two behaviors.

Anyways, I think __iter__ can be typed, in fact I believe I did that for the new Cursor object.

@Starbuck5 Starbuck5 added rect pygame.rect types Type hint checking related tasks labels Aug 18, 2021
@ankith26
Copy link
Contributor

ankith26 commented Dec 2, 2021

Related to this issue, Color is iterable but that is not typed as well. I am opening a misc typehints fix PR RN, but I will leave this issue for anyone else interested to tackle, maybe @Jumana-22 is interested to take this up (if not, I or someone else could do it as well)

@robertpfeiffer
Copy link
Contributor

is there a way to do destructuring assignment that isn't exposing an iterator?

@bkwabiad
Copy link

bkwabiad commented Dec 4, 2021

Hi, I would like to contribute to this issue, I can submit a PR with a misc typehints fix within 2 days if this is fine with @ankith26 and others?

@ankith26
Copy link
Contributor

ankith26 commented Dec 5, 2021

is there a way to do destructuring assignment that isn't exposing an iterator?

AFAIK that's not possible ATM with python

Hi, I would like to contribute to this issue, I can submit a PR with a misc typehints fix within 2 days if this is fine with @ankith26 and others?

Yeah, that would be cool for me! But I had recommended this issue to @Jumana-22 as well, so could you decide with them on who would work on this issue, so that we don't have 2 people working on the same issue :)

@bkwabiad
Copy link

bkwabiad commented Dec 5, 2021

Hi @Jumana-22, wondering if you would like to take on this issue. I believe I have a fix for this issue but if you would like to take this on I will contribute in other ways!

@novialriptide
Copy link
Contributor

It looks like this issue hasn't moved anywhere in a while. I'll start working on it.

@lordmauve
Copy link
Contributor

Did we reject the idea of making Rect not iterable, not quacking like a 4-item sequence? I think maybe there was a discussion on Discord about

(x, y) in rect

being allowed but always returning False, which can trip people up.

If people want the coordinates out we could add an attribute like .xywh

@ankith26
Copy link
Contributor

ankith26 commented Jan 3, 2022

Did we reject the idea of making Rect not iterable, not quacking like a 4-item sequence?

Hmm I just did some digging, and it turns out, Rect implements a subset of the Sequence protocol, and it does not even technically confirm to the Iterable ABC because the C code does not actually implement __iter__. Anyways, I think Rect should still behave like a Sequence, but the __iter__ method shouldn't be typed because it does not actually exist.

Also, (x, y) in rect is a recently added feature, it does Rect.contains now

@illume
Copy link
Member

illume commented Feb 6, 2022

(Removing the good first issue label because this one isn't so clear what there is to do)

@vnmabus
Copy link
Contributor Author

vnmabus commented Feb 6, 2022

Cannot Rect subclass typing.Sequence? It only requires the __getitem__ and __len__ methods, and it would add __contains__, __iter__, __reversed__, index, and count.

@ankith26
Copy link
Contributor

ankith26 commented Feb 6, 2022

No, IMHO we shouldn't make Rect subclass typing.Sequence. Having index and count on rect objects does not make sense.
Some notes from the linked PR that did not solve this issue, but are relevant to this issue

Objects that provide __getitem__ dunder (examples in pygame include Rect, Color, Vector and possibly more things) are practically Iterable but they don't actually confirm to collections.abc.Iterable if they don't provide an __iter__ method as also highlighted by the docs for the same.

The way to fix this is to implement an __iter__ function in C code for these objects, and also typehint it.

@lordmauve
Copy link
Contributor

The way to fix this is to implement an iter function in C code for these objects, and also typehint it.

That's a waste of effort and going in the wrong direction. I don't think we need an __iter__() for the same reason we don't need an index or a count. It's already iterable for practical purposes. The index-based iteration protocol isn't going away, it's just muddied by the much later introduction of type-based ABCs that can't represent this protocol.

@lordmauve
Copy link
Contributor

is there a way to do destructuring assignment that isn't exposing an iterator?

rect.xywh could return a tuple.

@ankith26
Copy link
Contributor

ankith26 commented Feb 8, 2022

That's a waste of effort and going in the wrong direction. I don't think we need an iter() for the same reason we don't need an index or a count. It's already iterable for practical purposes. The index-based iteration protocol isn't going away, it's just muddied by the much later introduction of type-based ABCs that can't represent this protocol.

As you already say, these objects are already iterable. Since pygame objects like vector, color, rect (and maybe other stuff IDK) implement a subset of the sequence protocol in the C code, many things already rely on these objects being iterable. Well it can be seen as a problem on the python end, because ATM, having a __getitem__ means that the object is already practically iterable but might not confirm to the ABCs. This is a python implementation detail that we have to live with. IMHO the most elegant solution would be to implement __iter__, I believe this would also have some good performance impact on code doing stuff like x, y, w, h = rect_obj, tuple(color_obj), list(vector_obj) and many such stuff like this that rely on these objects being iterable.
I do however, agree that we need not fully implement the Sequence protocol on these objects (because sequence protocol implies index and count methods), but all I am saying is that we should explicitly declare these as Iterable (which they already practically are)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rect pygame.rect types Type hint checking related tasks
Projects
None yet
8 participants