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

dgram: deprecate all previous private APIs #22011

Merged
merged 1 commit into from
Aug 7, 2018
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jul 28, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@cjihrig cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 28, 2018
@nodejs-github-bot nodejs-github-bot added the dgram Issues and PRs related to the dgram subsystem / UDP. label Jul 28, 2018

Type: Runtime

The `dgram` API previously contained a number of underscored APIs that were
Copy link
Member

@Trott Trott Jul 28, 2018

Choose a reason for hiding this comment

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

Nit: `dgram` API -> `dgram` module?

Copy link
Member

Choose a reason for hiding this comment

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

Nit: remove "a number of" or even "a number of underscored"?

Copy link
Contributor

@antsmartian antsmartian Jul 28, 2018

Choose a reason for hiding this comment

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

May be the below one :

The dgram module previously contained several APIs that were never meant to be accessed outside node core. These APIs include :

?

Copy link
Member

Choose a reason for hiding this comment

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

The dgram module previously contained several APIs that were never meant to be accessed outside node core. These APIs include :

@antsmartian That seems better to me, but a few nits:

  • node core -> Node.js core (The project is always referred to as Node.js. The executable is node.)

  • Maybe get rid of "These APIs include" because that suggests it's an incomplete list. Just put a colon after Node.js core and list the functions there?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Trott Yes that looks good to me. Thanks.

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

+1 on this change - given CITGM and the usual.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

PR-URL: nodejs#22011
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dgram Issues and PRs related to the dgram subsystem / UDP. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.