FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

docs: Clearly document the behavior of ee.once(). by lance · Pull Request #6371 · nodejs/node · GitHub

/ node Public

docs: Clearly document the behavior of ee.once(). - #6371

Closed
lance wants to merge 4 commits into
nodejs:masterfrom
lance:ee-once-docs
ConversationCommits (4)ChecksFiles changed
Closed

docs: Clearly document the behavior of ee.once().#6371
lance wants to merge 4 commits into
nodejs:masterfrom
lance:ee-once-docs

Conversation

lance commented Apr 25, 2016

Copy link
Copy Markdown
Member
Checklist
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

Addresses #5566. The ee.once() function is currently documented as
invoking the listener, and then removing it when the event is
triggered. However, this is not really the case. The listener is removed
and then invoked. This only matters in a narrow set of use cases, but
when it matters, it matters that the docs are correct.

See the issue (#5566) for a discussion on why the code has not been
modified to match the documentation, but instead the documentation has
been modified to match the code.

cjihrig reviewed Apr 25, 2016
View reviewed changes
Comment thread doc/api/events.md Outdated

cjihrig Apr 25, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

as it is called is still a bit ambiguous. Could you use before here?

cjihrig commented Apr 25, 2016

Copy link
Copy Markdown
Contributor

LGTM with a comment.

mscdex added doc Issues and PRs related to the documentations. events Issues and PRs related to the events subsystem / EventEmitter. labels Apr 25, 2016

eljefedelrodeodeljefe commented Apr 25, 2016

Copy link
Copy Markdown
Contributor

LGTM

addaleax commented Apr 25, 2016

Copy link
Copy Markdown
Member

LGTM but you may want to use doc: for your commit message; and if you want to reference the issue in it, I think it’s generally preferred to use the full github URL. You can also add an Fixes: header, I think.

lance commented Apr 25, 2016

Copy link
Copy Markdown
Member Author

@addaleax yeah, I committed with 'docs:' instead of 'doc:'. I was aware that Fixes: was available, but since I can't really close the issue, I just referenced it. Do you need these things to change before merging the PR?

jasnell commented Apr 25, 2016

Copy link
Copy Markdown
Member

@lance ... can you take a look at the new prependOnceListener() method description in the docs and see if it requires similar treatment?

addaleax commented Apr 25, 2016

Copy link
Copy Markdown
Member

@lance No, all of that can be taken care of when landing the commit. 😄

lance added 3 commits April 25, 2016 11:43
docs: Clearly document the behavior of ee.once().
28b9da8 
Addresses #5566. The `ee.once()` function is currently documented as
invoking the listener, and then removing it when the event is
triggered. However, this is not really the case. The listener is removed
and _then_ invoked. This only matters in a narrow set of use cases, but
when it matters, it matters that the docs are correct.

See the issue (#5566) for a discussion on why the code has not been
modified to match the documentation, but instead the documentation has
been modified to match the code.
doc: Clarity about timing for once listeners.
0fa1c90 
Based on comments from @cjihrig.
doc: Clarify behavior of ee.prependOnceListener.
dc1049f 
Additional documentation changes required for `ee.prependOnceListener`
as per #5566.

lance commented Apr 25, 2016

Copy link
Copy Markdown
Member Author

@jasnell took a look and updated the documentation. Based on my reading of lib/events.js it seems the behavior would be identical.

addaleax reviewed Apr 25, 2016
View reviewed changes
Comment thread doc/api/events.md Outdated
*beginning* of the listeners array. This listener is invoked only the next time
`eventName` is triggered, after which it is removed.
*beginning* of the listeners array. This listener is removed, and then invoked
only the next time `eventName` is triggered.

addaleax Apr 25, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

To me this sounds a bit like the listener is removed instantly after being added. Maybe just copy the corresponding sentence for .once() from above?

jasnell commented Apr 25, 2016

Copy link
Copy Markdown
Member

LGTM

jasnell added this to the 6.0.0 milestone Apr 25, 2016

ChALkeR commented Apr 25, 2016

Copy link
Copy Markdown
Member

LGTM

Fishrock123 removed this from the 6.0.0 milestone Apr 25, 2016
estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23
jasnell pushed a commit that referenced this pull request Apr 27, 2016
doc: correctly document the behavior of ee.once().
23818c6 
Addresses #5566. The `ee.once()` function is currently documented as
invoking the listener, and then removing it when the event is
triggered. However, this is not really the case. The listener is removed
and _then_ invoked. This only matters in a narrow set of use cases, but
when it matters, it matters that the docs are correct.

See the issue (#5566) for a discussion on why the code has not been
modified to match the documentation, but instead the documentation has
been modified to match the code.

Fixes: #5566
PR-URL: #6371
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

jasnell commented Apr 27, 2016

Copy link
Copy Markdown
Member

Landed in 23818c6.
Fixed up the commit message a bit.

jasnell closed this Apr 27, 2016
Fishrock123 pushed a commit that referenced this pull request May 4, 2016
doc: correctly document the behavior of ee.once().
36207c6 
Addresses #5566. The `ee.once()` function is currently documented as
invoking the listener, and then removing it when the event is
triggered. However, this is not really the case. The listener is removed
and _then_ invoked. This only matters in a narrow set of use cases, but
when it matters, it matters that the docs are correct.

See the issue (#5566) for a discussion on why the code has not been
modified to match the documentation, but instead the documentation has
been modified to match the code.

Fixes: #5566
PR-URL: #6371
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
This was referenced May 4, 2016
Closed
Merged

MylesBorins commented May 18, 2016

Copy link
Copy Markdown
Contributor

does not land cleanly on v4.x-staging. Would someone be willing to backport?

MylesBorins commented Jun 1, 2016

Copy link
Copy Markdown
Contributor

I'm going to mark this as don't land @lance / @nodejs/documentation please feel free to open a backport PR

lance mentioned this pull request Jun 2, 2016
Closed
2 tasks

lance commented Jun 2, 2016

Copy link
Copy Markdown
Member Author

@thealphanerd - submitted backport PR here #7103

I have not submitted anything for backport before. I hope it's all up to snuff.

MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
doc: Backport ee.once doc clarifications to 4.x.
6c475e5 
Addresses #5566. The `ee.once()` function is currently documented as
invoking the listener, and then removing it when the event is
triggered. However, this is not really the case. The listener is removed
and _then_ invoked. This only matters in a narrow set of use cases, but
when it matters, it matters that the docs are correct.

See the issue (#5566) for a discussion on why the code has not been
modified to match the documentation, but instead the documentation has
been modified to match the code.

Fixes: #5566
Ref: #6371
PR-URL: #7103
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 2, 2016
doc: Backport ee.once doc clarifications to 4.x.
250887f 
Addresses #5566. The `ee.once()` function is currently documented as
invoking the listener, and then removing it when the event is
triggered. However, this is not really the case. The listener is removed
and _then_ invoked. This only matters in a narrow set of use cases, but
when it matters, it matters that the docs are correct.

See the issue (#5566) for a discussion on why the code has not been
modified to match the documentation, but instead the documentation has
been modified to match the code.

Fixes: #5566
Ref: #6371
PR-URL: #7103
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
doc: Backport ee.once doc clarifications to 4.x.
3b46187 
Addresses #5566. The `ee.once()` function is currently documented as
invoking the listener, and then removing it when the event is
triggered. However, this is not really the case. The listener is removed
and _then_ invoked. This only matters in a narrow set of use cases, but
when it matters, it matters that the docs are correct.

See the issue (#5566) for a discussion on why the code has not been
modified to match the documentation, but instead the documentation has
been modified to match the code.

Fixes: #5566
Ref: #6371
PR-URL: #7103
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
doc: Backport ee.once doc clarifications to 4.x.
baa787b 
Addresses #5566. The `ee.once()` function is currently documented as
invoking the listener, and then removing it when the event is
triggered. However, this is not really the case. The listener is removed
and _then_ invoked. This only matters in a narrow set of use cases, but
when it matters, it matters that the docs are correct.

See the issue (#5566) for a discussion on why the code has not been
modified to match the documentation, but instead the documentation has
been modified to match the code.

Fixes: #5566
Ref: #6371
PR-URL: #7103
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. events Issues and PRs related to the events subsystem / EventEmitter.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL