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

Remove event.peek() behavior, minor doc and stub fixes #3122

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

zoldalma999
Copy link
Member

Fixes #1196 by documenting the behavior of pygame.event.peek if nothing is passed in.

Also marks event.type and event.dict readonly in the stubs, adds runtime docs to them, and makes the pump argument more accurate in the stubs.

@zoldalma999 zoldalma999 requested a review from a team as a code owner September 25, 2024 20:12
Copy link
Contributor

@bilhox bilhox left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the PR!
Worth to mention that @gresm also proposed a fix for this issue in his latest PR.
Otherwise, I left little changes.

docs/reST/ref/event.rst Outdated Show resolved Hide resolved
docs/reST/ref/event.rst Outdated Show resolved Hide resolved
@yunline yunline added docs type hints Type hint checking related tasks event pygame.event labels Sep 26, 2024
Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

If this is becoming public, a test or two for it would also be good

exclude: Optional[_EventTypes] = None,
) -> List[Event]: ...
def poll() -> Event: ...
def wait(timeout: int = 0) -> Event: ...
def peek(eventtype: Optional[_EventTypes] = None, pump: Any = True) -> bool: ...
def clear(eventtype: Optional[_EventTypes] = None, pump: Any = True) -> None: ...
def peek(eventtype: Optional[_EventTypes] = None, pump: bool = True) -> bool: ...
Copy link
Member

Choose a reason for hiding this comment

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

The return value probably needs to be updated here to be consistent with the docs.

Also, the reason it was typed as Any in the first place is because almost any object in python is acceptable as a bool, as you probably already know

@zoldalma999
Copy link
Member Author

If this is becoming public, a test or two for it would also be good

Wanted to write one for it, but got a segfault with this code:

import pygame

pygame.init()
pygame.event.clear()

e = pygame.Event(pygame.KEYDOWN, key=pygame.K_SPACE)
pygame.event.post(e)

print("1")
assert e == pygame.event.peek(pump=False)
print("2")
assert e == pygame.event.peek(None, False)
print("3") # This one never gets printed

Debugging it a bit more, seems like the dict proxy on this line is already freed

pgEventDictProxy *dict_proxy = (pgEventDictProxy *)event->user.data1;

@ankith26
Copy link
Member

After giving it a bit of thought, my current opinion is that we should remove this functionality all together now.

  • If it is segfaulting right now and no one has reported it yet, no one knows or cares about this behaviour.
  • For whatever reason if we want to document it down the line, it can be readded and fixed then.

@bilhox bilhox added this to the 2.5.3 milestone Oct 13, 2024
@zoldalma999 zoldalma999 changed the title Document event.peek() behavior, minor doc and stub fixes Remove event.peek() behavior, minor doc and stub fixes Dec 4, 2024
Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

I think there is a bit of confusion here, what I actually wanted to convey was "make the return value a bool regardless of the input, just remove the behaviour of returning an event type". I would still like to retain the None/optional-arg case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs event pygame.event type hints Type hint checking related tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pygame.event.peek docs missing a detail (2329)
4 participants