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.

Sign up for GitHub

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

Jump to bottom

net: prefer === to == #11513

Closed
wants to merge 1 commit into from
Closed

net: prefer === to == #11513

wants to merge 1 commit into from

Conversation

notarseniy
Copy link
Contributor

@notarseniy notarseniy commented Feb 22, 2017 •
edited
Loading

  • Prefer === to == in one condition
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

net

@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Feb 22, 2017
@TimothyGu
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/6555/

@vkurchatkin vkurchatkin added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 23, 2017
jasnell
jasnell previously approved these changes Feb 23, 2017
View reviewed changes
@vkurchatkin
Copy link
Contributor

This is a serious breaking change

@jasnell
Copy link
Member

jasnell commented Feb 23, 2017

ah, yes, good point. The status change should be ok, but the options.fd change is not.

@jasnell jasnell dismissed their stale review February 23, 2017 00:28

Second thoughts... needs some tweaking to avoid the breaking change

@notarseniy
Copy link
Contributor Author

notarseniy commented Feb 23, 2017 •
edited
Loading

Sorry to ask this, but can someone explain what exactly is the breakdown? It seems that I something misunderstood :( What the breaking case?

Trott
Trott reviewed Feb 23, 2017
View reviewed changes
lib/net.js Outdated
@@ -146,7 +146,7 @@ function Socket(options) {
} else if (options.fd !== undefined) {
this._handle = createHandle(options.fd);
this._handle.open(options.fd);
if ((options.fd == 1 || options.fd == 2) &&
if ((options.fd === 1 || options.fd === 2) &&
Copy link
Member

@Trott Trott Feb 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

If I'm following the code correctly, this could conceivably be a breaking change for some. Since options.fd can be provided by the end user, it may be a string. Before this change, passing in '1' would have the same effect as 1. But now it won't. It only affects whether stdout and stderr are blocking or not on Windows. ¯\(ツ)/¯ @nodejs/platform-windows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, got it. Thank you!
Hmm, and what are workarounds? ParseInt'ing? (looks like hack :( ) Leave it as is? (options.fd can be === true, thats not okay?)

Copy link
Member

Choose a reason for hiding this comment

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

Well, you could just leave ==. :-D

options.fd = +options.fd should make it a number too. There are also bit-shifting things I think that might be more efficient.

But ultimately, I'm not sure this needs to be changed at all.

@notarseniy notarseniy force-pushed the net-minor-refactor branch 3 times, most recently from e59eaa4 to 89fa167 Compare February 24, 2017 04:21
@notarseniy
Copy link
Contributor Author

Reverted options.fd-related corrections. Added comment before if-statements for explaining these two non-strict comparisons with link to this PR.

semver-major label now can be removed. Thanks to @vkurchatkin for pointing this out.

@Trott Trott removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 24, 2017
cjihrig
cjihrig approved these changes Feb 24, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with a nit.

lib/net.js Outdated
@@ -146,6 +146,8 @@ function Socket(options) {
} else if (options.fd !== undefined) {
this._handle = createHandle(options.fd);
this._handle.open(options.fd);
// Non-strict equations here for legacy reasons.
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of saying this and linking to a GitHub issue, could you just explain the legacy reasons.

@notarseniy
net: prefer === to ==
9209efa 
* Change === to == in one place
* Add explanation about another non-strict if-statement
@notarseniy
Copy link
Contributor Author

Expanded commit message; made comment on options.fd more better.

@jasnell
Copy link
Member

jasnell commented Feb 25, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/6582/

jasnell pushed a commit that referenced this pull request Feb 27, 2017
@notarseniy @jasnell
net: prefer === to ==
84c448e 
* Change === to == in one place
* Add explanation about another non-strict if-statement

PR-URL: #11513
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member

jasnell commented Feb 27, 2017

Landed in 84c448e

@jasnell jasnell closed this Feb 27, 2017
@notarseniy notarseniy deleted the net-minor-refactor branch February 27, 2017 22:54
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 28, 2017
@notarseniy @italoacasas
net: prefer === to ==
3afe90d 
* Change === to == in one place
* Add explanation about another non-strict if-statement

PR-URL: nodejs#11513
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@italoacasas italoacasas mentioned this pull request Feb 28, 2017
Merged
jasnell pushed a commit that referenced this pull request Mar 7, 2017
@notarseniy @jasnell
net: prefer === to ==
40833a4 
* Change === to == in one place
* Add explanation about another non-strict if-statement

PR-URL: #11513
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
@notarseniy @jasnell
net: prefer === to ==
7d0f808 
* Change === to == in one place
* Add explanation about another non-strict if-statement

PR-URL: #11513
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@notarseniy @MylesBorins
net: prefer === to ==
742ec62 
* Change === to == in one place
* Add explanation about another non-strict if-statement

PR-URL: #11513
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@notarseniy @MylesBorins
net: prefer === to ==
a7df345 
* Change === to == in one place
* Add explanation about another non-strict if-statement

PR-URL: #11513
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@notarseniy @TimothyGu @vkurchatkin @jasnell @Trott @cjihrig @nodejs-github-bot