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

EventListener::new footgun #89

Closed
zeenix opened this issue Oct 17, 2023 · 1 comment · Fixed by #90
Closed

EventListener::new footgun #89

zeenix opened this issue Oct 17, 2023 · 1 comment · Fixed by #90

Comments

@zeenix
Copy link
Member

zeenix commented Oct 17, 2023

Copy&pasting from the matrix channel to avoid repeating:

zeenix: this method takes an Event ref but doesn't actually setup the listener to listen to this event. This seems like a pretty big footgun
Saltbag: The only other option is to have it insert itself into the list on await, which has its own footguns. It can’t be set up in new since it needs to be pinned.
zeenix: it's just a very strange and unintuitive api now. I see multiple issues here:

  • EventListener existing w/o listening is just strange. The name implies something that's listening.
  • new takes a Event ref but doesn't listen to the given event.
  • The footgun of awaiting a listener w/o calling listen on it. That state should not be allowed to ever exist.

If listening require pinning, that should always be implied then. That's the whole point of EventListener type. At the very least, new and EventListener docs should document all these caveats well.

@MaxVerevkin
Copy link

The only other option is to have it insert itself into the list on await, which has its own footguns

Is it because some may assume that listener starts listening immediately after being created when in reality it will listen only when awaited/.wait()'ed?

notgull added a commit that referenced this issue Oct 18, 2023
In retrospect it is sometimes unclear in the documentation that the new event
listener needs to be pinned and inserted into the list before it can receive
events. This PR adds documentation that should clarify this issue.

Closes #89

Signed-off-by: John Nunley <dev@notgull.net>
notgull added a commit that referenced this issue May 4, 2024
In retrospect it is sometimes unclear in the documentation that the new event
listener needs to be pinned and inserted into the list before it can receive
events. This PR adds documentation that should clarify this issue.

Closes #89

Signed-off-by: John Nunley <dev@notgull.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants