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

Jump to bottom

net: move isLegalPort to internal/net #4882

Merged
merged 1 commit into from Jan 30, 2016
Merged

net: move isLegalPort to internal/net #4882

merged 1 commit into from Jan 30, 2016

Conversation

evanlucas
Copy link
Contributor

isLegalPort can be used in more places than just net.js. This change
moves it to a new internal net module in preparation for using it in
the dns module.

@evanlucas evanlucas added the net Issues and PRs related to the net subsystem. label Jan 26, 2016
@evanlucas evanlucas mentioned this pull request Jan 26, 2016
Closed
@cjihrig
Copy link
Contributor

cjihrig commented Jan 26, 2016

LGTM

bnoordhuis
bnoordhuis reviewed Jan 26, 2016
View reviewed changes
test/parallel/test-net-internal.js
assert.strictEqual(net.isLegalPort(65536), false);
assert.strictEqual(net.isLegalPort('65535'), true);
assert.strictEqual(net.isLegalPort(undefined), false);
assert.strictEqual(net.isLegalPort(null), true);
Copy link
Member

Choose a reason for hiding this comment

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

The difference between undefined and null seems a bit incongruous in retrospect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Would it be worth it to move the null/undefined checks to this function? They are currently done in net in both places where it is being used. Seems like we could simplify some logic by doing so

Copy link
Member

Choose a reason for hiding this comment

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

Good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bnoordhuis is that something you would prefer I do here or in another PR?

Copy link
Member

Choose a reason for hiding this comment

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

A separate commit in this PR is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It appears to be a bit more than I expected. Mind if I wait until another PR?

@bnoordhuis
Copy link
Member

LGTM

jasnell
jasnell reviewed Jan 27, 2016
View reviewed changes
lib/internal/net.js
@@ -0,0 +1,11 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

Is a new module file necessary? This seems small enough that it could possibly just go into internal/util

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasnell While it is small enough now for that, I have been working on cleaning up some of the logic in net and moving some more into internal. Either way works for me though, so if this is something you feel strongly about, I'll be happy to change.

Copy link
Member

Choose a reason for hiding this comment

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

@evanlucas ... :-) then by all means, continue as you are!

@jasnell
Copy link
Member

jasnell commented Jan 27, 2016

Minor nit but overall LGTM

thefourtheye
thefourtheye reviewed Jan 29, 2016
View reviewed changes
lib/internal/net.js
function isLegalPort(port) {
if (typeof port === 'string' && port.trim() === '')
return false;
return +port === (port >>> 0) && port >= 0 && port <= 0xFFFF;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to use Number.isInteger here?

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with parseInt() is that something like '10c' becomes 10, which is not right. The problem with Number.isInteger() is that '0xff' (or any other string) is false. There are all kinds of weird edge cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

True. Should we really allow values like that? Coercion allows people to use a lot of undocumented allowed values like this and I am not sure if we can cover all the possible valid values without a lot of checks. For example this condition would allow zero to one element number arrays.

Copy link
Contributor

Choose a reason for hiding this comment

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

My personal opinion - no we shouldn't coerce. But, it would be an unnecessary breaking change. Also, this function is used for range checking, not type checking.

@evanlucas
Copy link
Contributor Author

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

@thefourtheye
Copy link
Contributor

LGTM

@evanlucas
net: move isLegalPort to internal/net
6cbbfef 
isLegalPort can be used in more places than just net.js. This change
moves it to a new internal net module in preparation for using it in
the dns module.

PR-URL: nodejs#4882
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@evanlucas evanlucas closed this Jan 30, 2016
@evanlucas evanlucas deleted the dnsnet-internal branch January 30, 2016 23:57
@evanlucas
Copy link
Contributor Author

Landed in 6cbbfef. Thanks

@evanlucas evanlucas merged commit 6cbbfef into nodejs:master Jan 30, 2016
rvagg pushed a commit that referenced this pull request Feb 8, 2016
@evanlucas @rvagg
net: move isLegalPort to internal/net
c9b05da 
isLegalPort can be used in more places than just net.js. This change
moves it to a new internal net module in preparation for using it in
the dns module.

PR-URL: #4882
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@rvagg rvagg mentioned this pull request Feb 8, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
@evanlucas
net: move isLegalPort to internal/net
76e07c9 
isLegalPort can be used in more places than just net.js. This change
moves it to a new internal net module in preparation for using it in
the dns module.

PR-URL: #4882
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
@evanlucas
net: move isLegalPort to internal/net
96f306f 
isLegalPort can be used in more places than just net.js. This change
moves it to a new internal net module in preparation for using it in
the dns module.

PR-URL: #4882
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 18, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
@evanlucas
net: move isLegalPort to internal/net
6062b1c 
isLegalPort can be used in more places than just net.js. This change
moves it to a new internal net module in preparation for using it in
the dns module.

PR-URL: #4882
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@evanlucas
net: move isLegalPort to internal/net
50f46d8 
isLegalPort can be used in more places than just net.js. This change
moves it to a new internal net module in preparation for using it in
the dns module.

PR-URL: nodejs#4882
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
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
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.

None yet
6 participants
@evanlucas @cjihrig @bnoordhuis @jasnell @thefourtheye @MylesBorins