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

Document that EventListeners must be listen'd on #90

Merged
merged 2 commits into from
Oct 19, 2023
Merged

Conversation

notgull
Copy link
Member

@notgull notgull commented 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
cc @zeenix

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 notgull changed the title docs: Document that EventListeners must be listen'd on Document that EventListeners must be listen'd on Oct 18, 2023
Copy link
Member

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

Thanks for the quick action here and adding such detailed docs! 👍 However, apart from the nit comment, I think we should also at least change the API (in a compatible way) to either add a new new method that doesn't take a Event ref and deprecate new or (if possible) make listen() implicit in the first time EventListener is polled and deprecate EventListener::listen. I'd very much prefer the latter, since it removes the footgun.

src/lib.rs Outdated Show resolved Hide resolved
@zeenix
Copy link
Member

zeenix commented Oct 18, 2023

think we should also at least change the API (in a compatible way) to either add a new new method that doesn't take a Event ref and deprecate new or (if possible) make listen() implicit in the first time EventListener is polled and deprecate EventListener::listen. I'd very much prefer the latter, since it removes the footgun.

I created #91 for this part so you can ignore this comment here.

Signed-off-by: John Nunley <dev@notgull.net>
@notgull notgull requested a review from zeenix October 19, 2023 00:18
Copy link
Member

@zeenix zeenix 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 again.

@zeenix zeenix merged commit e8a1727 into master Oct 19, 2023
9 checks passed
@zeenix zeenix deleted the notgull/new-docs branch October 19, 2023 10:49
@notgull notgull mentioned this pull request Oct 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

EventListener::new footgun
2 participants