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

net: move isLegalPort to internal/net by evanlucas · Pull Request #4882 · nodejs/node · GitHub

/ node Public

net: move isLegalPort to internal/net - #4882

Merged
evanlucas merged 1 commit into
nodejs:masterfrom
evanlucas:dnsnet-internal
Jan 30, 2016
ConversationCommits (1)ChecksFiles changed
Merged

net: move isLegalPort to internal/net#4882
evanlucas merged 1 commit into
nodejs:masterfrom
evanlucas:dnsnet-internal

Conversation

evanlucas commented Jan 26, 2016

Copy link
Copy Markdown
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 added the net Issues and PRs related to the net subsystem. label Jan 26, 2016
evanlucas mentioned this pull request Jan 26, 2016
Closed

cjihrig commented Jan 26, 2016

Copy link
Copy Markdown
Contributor

LGTM

bnoordhuis reviewed Jan 26, 2016
View reviewed changes
Comment thread test/parallel/test-net-internal.js

bnoordhuis Jan 26, 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

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

evanlucas Jan 26, 2016

Copy link
Copy Markdown
Contributor Author

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

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

bnoordhuis Jan 26, 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

Good idea.

evanlucas Jan 26, 2016

Copy link
Copy Markdown
Contributor Author

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

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

bnoordhuis Jan 26, 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

A separate commit in this PR is fine.

evanlucas Jan 29, 2016

Copy link
Copy Markdown
Contributor Author

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

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

bnoordhuis commented Jan 26, 2016

Copy link
Copy Markdown
Member

LGTM

jasnell reviewed Jan 27, 2016
View reviewed changes
Comment thread lib/internal/net.js

jasnell Jan 27, 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

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

evanlucas Jan 28, 2016

Copy link
Copy Markdown
Contributor Author

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

@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.

jasnell Jan 28, 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

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

jasnell commented Jan 27, 2016

Copy link
Copy Markdown
Member

Minor nit but overall LGTM

thefourtheye reviewed Jan 29, 2016
View reviewed changes
Comment thread lib/internal/net.js

thefourtheye Jan 29, 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

Would it make sense to use Number.isInteger here?

cjihrig Jan 29, 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

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.

thefourtheye Jan 29, 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

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.

cjihrig Jan 29, 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

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 commented Jan 30, 2016

Copy link
Copy Markdown
Contributor Author

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

thefourtheye commented Jan 30, 2016

Copy link
Copy Markdown
Contributor

LGTM

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 closed this Jan 30, 2016
evanlucas deleted the dnsnet-internal branch January 30, 2016 23:57

evanlucas commented Jan 30, 2016

Copy link
Copy Markdown
Contributor Author

Landed in 6cbbfef. Thanks

evanlucas merged commit 6cbbfef into nodejs:master Jan 30, 2016
rvagg pushed a commit that referenced this pull request Feb 8, 2016
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 mentioned this pull request Feb 8, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
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
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 mentioned this pull request Feb 18, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
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
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>
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

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.

6 participants

Back | FazBrowse Home | New Git URL