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

doc: change eventName type annotations #17666

Closed
wants to merge 2 commits into from

Conversation

aprilwebster
Copy link
Contributor

Change type annotations for eventName in events API doc from {any} to
{string|symbol} to be consistent with doc changes made in #17440.

Fixes: #17657

Checklist
Affected core subsystem(s)

doc

@addaleax
Copy link
Member

@addaleax addaleax added doc Issues and PRs related to the documentations. events Issues and PRs related to the events subsystem / EventEmitter. labels Dec 13, 2017
Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

@evanlucas evanlucas 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 helping make our documentation better!

@apapirovski
Copy link
Member

Now that #17440 landed, this will need to be rebased and rawListeners also needs to be updated. Thanks!

@apapirovski apapirovski reopened this Dec 14, 2017
@aprilwebster
Copy link
Contributor Author

Sure, np! Can take care of that today.

@lpinca
Copy link
Member

lpinca commented Dec 14, 2017

If I remember correctly @mscdex was not in favor of this due to numbers being "valid" or something like that.

@apapirovski
Copy link
Member

@lpinca I mean, that's sort of what I brought up in the original thread. Right now you can enter absolutely anything and it'll just be coerced to a string (if it's not a Symbol).

@lpinca
Copy link
Member

lpinca commented Dec 14, 2017

Yes, I have nothing against this change, I'm actually +1 just wanted to get @mscdex attention as they might be interested/concerned.

Change type annotations for eventName in events API doc from {any} to {string|symbol} to be consistent with doc changes made in nodejs#17440.

Fixes: nodejs#17657
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 17, 2017
@apapirovski
Copy link
Member

Landed in b2432a7. Thanks for the contribution @aprilwebster and congrats on becoming a Contributor!! 🎉

apapirovski pushed a commit that referenced this pull request Dec 17, 2017
Change type annotations for eventName in events API doc
from {any} to {string|symbol}.

PR-URL: #17666
Fixes: #17657
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@aprilwebster
Copy link
Contributor Author

Thanks @apapirovski!! Hopefully I can make a few more contributions in the new year. Happy holidays!

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 29, 2017
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
Change type annotations for eventName in events API doc
from {any} to {string|symbol}.

PR-URL: #17666
Fixes: #17657
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
Change type annotations for eventName in events API doc
from {any} to {string|symbol}.

PR-URL: #17666
Fixes: #17657
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.