events: - line #354 - comment typo. "iff" corrected #13496

jonsey247 · 2017-06-06T11:39:07Z

Simple typo. line 354 in events.js file. "iff" instead of "if". This could be a cause of confusion.

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

[refack fixed formating]

BridgeAR · 2017-06-06T12:02:17Z

I think this was intentional and is correct the way it is: https://en.wikipedia.org/wiki/If_and_only_if

mscdex · 2017-06-06T12:07:55Z

Yep, @BridgeAR is correct here, it's written that way on purpose.

refack · 2017-06-06T12:14:04Z

lib/events.js

@@ -351,7 +351,7 @@ EventEmitter.prototype.prependOnceListener =
      return this;
    };

-// emits a 'removeListener' event iff the listener was removed
+// emits a 'removeListener' event if the listener was removed


Probably best to change to if and only if

I agree. This isn't the first time an issue like this has been opened.

refack · 2017-06-06T12:31:28Z

@jonsey247 Thank you for the contribution, and welcome 🥇. Even if this change isn't adopted we appreciate the feedback.

jonsey247 · 2017-06-06T13:05:18Z

Thanks @refack. Plus side, I learnt what "iff" means 😃

Trott · 2017-06-06T16:01:54Z

@jonsey247 Do you want to try to update this to change it to if and only if or would you prefer it be closed?

Trott · 2017-06-06T16:03:05Z

FWIW, that's the only instance of iff in the lib directory so it might make sense to expand it to if and only if. While iff is a convention, it's not one we use otherwise in that area of the code.

jonsey247 · 2017-06-06T16:30:29Z

hi @Trott yes i can do that.

refack

💯

jonsey247

iff change to "if and only if"

jonsey247 · 2017-06-06T16:38:00Z

This is officially my first "contribution" to an open-source project! 😂 👍 Thanks @refack

refack · 2017-06-06T16:39:25Z

Hit the wrong button?

jonsey247 · 2017-06-06T16:41:30Z

.....yep

Trott

LGTM

IMO this doesn't need to wait the usual 48 hours.

CI might be unnecessary but better safe than sorry? At a minimum, whoever lands this should run the linter to make sure there isn't something surprising (like a new rule that slipped in that somehow would flag this for some reason I can't even think of)

Trott · 2017-06-06T17:01:32Z

lib/events.js

@@ -351,7 +351,7 @@ EventEmitter.prototype.prependOnceListener =
      return this;
    };

-// emits a 'removeListener' event iff the listener was removed
+// emits a 'removeListener' event if and only if the listener was removed


Totally optional, but while you're in here, if you want, emits -> Emits and put a . at the end. I'm pretty sure we have Collaborators that strongly prefer that style (especially the capitalization), but I also know we have lots of comments that don't conform to it. So totally OK if you don't do it.

so it would read:
emits -> Emits a 'removeListener' event if and only if the listener was removed.
??

Yes. (And like I said: Totally optional!)

(For that matter, it's something that whoever lands the PR can fix up if they want to.)

@jonsey247 I'll land this. Comment if you want me to fix the sentence?

refack · 2017-06-06T17:02:58Z

Linter CI: https://ci.nodejs.org/job/node-test-linter/9653/

…dejs#354 -emits -> Emits a removeListener event if and only if the listener was removed.

jonsey247

changes to "emits -> Emits a 'removeListener' event if and only if the listener was removed.". Capitalisation and full-stop added.

refack · 2017-06-06T17:56:31Z

lib/events.js

@@ -351,7 +351,7 @@ EventEmitter.prototype.prependOnceListener =
      return this;
    };

-// emits a 'removeListener' event if and only if the listener was removed
+// emits -> Emits a 'removeListener' event if and only if the listener was removed.


Almost 😄
I believe what @Trott ment with the arrow is emits needs to be replaced with Emits.
I'll fix this.

😂 Thank you. And thank you for putting up with me!

Trott · 2017-06-06T17:57:01Z

lib/events.js

@@ -351,7 +351,7 @@ EventEmitter.prototype.prependOnceListener =
      return this;
    };

-// emits a 'removeListener' event iff the listener was removed
+// emits -> Emits a 'removeListener' event if and only if the listener was removed.


Sorry! Get rid of emits ->.

When I typed this:

emits -> Emits

...the -> was meant as an arrow and it meant "Change the thing on the left to be the thing on the right instead". Sorry about the confusion.

Oh, I see @refack said he'll fix it. Cool.

It's ok. I can change it now, unless @refack has changed it?

…dejs#354 Emits a removeListener event if and only if the listener was removed.

refack · 2017-06-06T18:05:40Z

@jonsey247 One more thing, in the git log you appear as:
Author: jonsey247 <jacobfilmjones@hotmail.com>
Is that Ok, or would you like your full name to be recorded?
https://help.github.com/articles/setting-your-username-in-git/

jonsey247 · 2017-06-06T18:11:35Z

No problem @refack (been meaning to do that). Changed now. 👍

mhdawson

LGTM

refack · 2017-06-06T18:16:30Z

No problem @refack (been meaning to do that). Changed now. 👍

You'll need to rebase, or add another empty commit https://git-scm.com/docs/git-commit#git-commit---allow-empty

jonsey247 · 2017-06-06T18:31:02Z

@refack......not sure if I've done that.

PR-URL: nodejs#13496 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

refack · 2017-06-06T18:40:26Z

Landed in afc59a9
Extra linter run on master: https://ci.nodejs.org/job/node-test-linter/9657/

PR-URL: #13496 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

events: - line nodejs#354 - comment typo. "iff" corrected

4cc31c0

nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Jun 6, 2017

refack reviewed Jun 6, 2017

View reviewed changes

events: - line nodejs#354 - "iff" changed to "if and only if".

ca72caf

refack approved these changes Jun 6, 2017

View reviewed changes

jonsey247 commented Jun 6, 2017

View reviewed changes

jonsey247 closed this Jun 6, 2017

refack reopened this Jun 6, 2017

Trott approved these changes Jun 6, 2017

View reviewed changes

cjihrig approved these changes Jun 6, 2017

View reviewed changes

Trott reviewed Jun 6, 2017

View reviewed changes

refack self-assigned this Jun 6, 2017

events: - line nodejs#354 - if and only if updated. events: - line no…

02e4a4b

…dejs#354 -emits -> Emits a removeListener event if and only if the listener was removed.

jonsey247 commented Jun 6, 2017

View reviewed changes

refack reviewed Jun 6, 2017

View reviewed changes

Trott reviewed Jun 6, 2017

View reviewed changes

events: - line nodejs#354 - if and only if updated. events: - line no…

10631db

…dejs#354 Emits a removeListener event if and only if the listener was removed.

mhdawson approved these changes Jun 6, 2017

View reviewed changes

-m'no change'

15ec99b

refack closed this Jun 6, 2017

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

MylesBorins mentioned this pull request Jul 18, 2017

v6.11.2 proposal #14356

Merged

refack removed their assignment Oct 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

events: - line #354 - comment typo. "iff" corrected #13496

events: - line #354 - comment typo. "iff" corrected #13496

jonsey247 commented Jun 6, 2017 •

edited by refack

Loading

BridgeAR commented Jun 6, 2017

mscdex commented Jun 6, 2017 •

edited

Loading

refack Jun 6, 2017

cjihrig Jun 6, 2017

refack commented Jun 6, 2017

jonsey247 commented Jun 6, 2017

Trott commented Jun 6, 2017

Trott commented Jun 6, 2017

jonsey247 commented Jun 6, 2017

refack left a comment

jonsey247 left a comment

jonsey247 commented Jun 6, 2017

refack commented Jun 6, 2017

jonsey247 commented Jun 6, 2017

Trott left a comment

Trott Jun 6, 2017

jonsey247 Jun 6, 2017

Trott Jun 6, 2017

Trott Jun 6, 2017

refack Jun 6, 2017 •

edited

Loading

refack commented Jun 6, 2017

jonsey247 left a comment •

edited

Loading

refack Jun 6, 2017

jonsey247 Jun 6, 2017

Trott Jun 6, 2017

Trott Jun 6, 2017

jonsey247 Jun 6, 2017

refack commented Jun 6, 2017

jonsey247 commented Jun 6, 2017

mhdawson left a comment

refack commented Jun 6, 2017

jonsey247 commented Jun 6, 2017

refack commented Jun 6, 2017

events: - line #354 - comment typo. "iff" corrected #13496

events: - line #354 - comment typo. "iff" corrected #13496

Conversation

jonsey247 commented Jun 6, 2017 • edited by refack Loading

Checklist

Affected core subsystem(s)

BridgeAR commented Jun 6, 2017

mscdex commented Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jun 6, 2017

jonsey247 commented Jun 6, 2017

Trott commented Jun 6, 2017

Trott commented Jun 6, 2017

jonsey247 commented Jun 6, 2017

refack left a comment

Choose a reason for hiding this comment

jonsey247 left a comment

Choose a reason for hiding this comment

jonsey247 commented Jun 6, 2017

refack commented Jun 6, 2017

jonsey247 commented Jun 6, 2017

Trott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

refack commented Jun 6, 2017

jonsey247 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jun 6, 2017

jonsey247 commented Jun 6, 2017

mhdawson left a comment

Choose a reason for hiding this comment

refack commented Jun 6, 2017

jonsey247 commented Jun 6, 2017

refack commented Jun 6, 2017

jonsey247 commented Jun 6, 2017 •

edited by refack

Loading

mscdex commented Jun 6, 2017 •

edited

Loading

refack Jun 6, 2017 •

edited

Loading

jonsey247 left a comment •

edited

Loading