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

module: deprecate Module._debug by JacksonTian · Pull Request #13948 · nodejs/node · GitHub

/ node Public

module: deprecate Module._debug - #13948

Closed
JacksonTian wants to merge 1 commit into
nodejs:masterfrom
JacksonTian:_debug
ConversationCommits (1)ChecksFiles changed
Closed

module: deprecate Module._debug#13948
JacksonTian wants to merge 1 commit into
nodejs:masterfrom
JacksonTian:_debug

Conversation

JacksonTian commented Jun 27, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

The _debug of Module is undocumented and it useless here.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

module

nodejs-github-bot added the module Issues and PRs related to the module subsystem. label Jun 27, 2017

jasnell commented Jun 27, 2017

Copy link
Copy Markdown
Member

This one has been there for so long we might need to do a deprecation cycle on it. Marking semver-major defensively. Ping @nodejs/ctc

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 27, 2017

jasnell commented Jun 27, 2017

Copy link
Copy Markdown
Member

(to clarify, I would highly doubt that it's being used, but it pays to be safe)

BridgeAR commented Aug 30, 2017

Copy link
Copy Markdown
Member

This needs some more reviews from @nodejs/ctc

BridgeAR commented Aug 30, 2017

Copy link
Copy Markdown
Member

@jasnell I guess it is safe to land this as is?

jasnell commented Aug 30, 2017

Copy link
Copy Markdown
Member

Perhaps we should ping @ChALkeR and ask him to please please please do a quick ecosystem search, just to be safe. We should also do a CITGM run just in case. It also needs CI

BridgeAR commented Aug 30, 2017

Copy link
Copy Markdown
Member

CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/962
CI https://ci.nodejs.org/job/node-test-pull-request/9890/

evanlucas previously requested changes Aug 30, 2017
View reviewed changes

evanlucas left a comment

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

Can we deprecate this like we normally do? It shouldn't cost anything. If it does, we will get complaints which will show it is being used.

mscdex commented Aug 30, 2017

Copy link
Copy Markdown
Contributor

It wouldn't hurt to deprecate it.

jasnell commented Aug 31, 2017

Copy link
Copy Markdown
Member

Yep, I'm good with a regular deprecation.

JacksonTian commented Aug 31, 2017

Copy link
Copy Markdown
Contributor Author

Thanks all. I will change to deprecate it first.

JacksonTian force-pushed the _debug branch 2 times, most recently from e35b098 to 26c981e Compare September 2, 2017 08:27
JacksonTian changed the title module: remove the intermediate Module._debug module: deprecate Module._debug Sep 2, 2017

JacksonTian commented Sep 2, 2017

Copy link
Copy Markdown
Contributor Author

Hi everyone, I updated this PR with deprecation.

TimothyGu approved these changes Sep 2, 2017
View reviewed changes

TimothyGu left a comment

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

Lgtm besides the nits.

Comment thread doc/api/deprecations.md Outdated

TimothyGu Sep 2, 2017

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

Would you mind escaping the underscore?

TimothyGu Sep 2, 2017

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

Also the DEP ID is off.

jasnell reviewed Sep 5, 2017
View reviewed changes
Comment thread doc/api/deprecations.md Outdated

jasnell Sep 5, 2017

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

This should actually be DEP00XX until it lands. The person landing needs to assign the specific code.

JacksonTian Sep 6, 2017

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

And the

Module._debug = util.deprecate(debug, 'Module._debug is deprecated.',
                               'DEP0076');

should be DEP00XX also?

jasnell Sep 6, 2017

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

Yeah, all of the DEP00.. identifiers.

module: deprecate Module._debug
a381a7f 
The _debug of Module is undocumented and it useless here.

JacksonTian commented Sep 9, 2017

Copy link
Copy Markdown
Contributor Author

Hi all, I have updated this PR.

BridgeAR commented Sep 12, 2017

Copy link
Copy Markdown
Member

@evanlucas the PR was updated with your suggestion. PTAL

thefourtheye commented Sep 12, 2017

Copy link
Copy Markdown
Contributor

Our deprecation guidelines don't cover this. So I'll just ask here. Do we allow an undocumented API to be runtime deprecated directly without documenting and deprecating first?

BridgeAR commented Sep 13, 2017

Copy link
Copy Markdown
Member

@thefourtheye that is a good point. I guess we should go ahead and improve the deprecation guidelines. I personally do not feel like it is the right thing to do to document a so far undocumented feature with the goal of actually removing it at some point as that is somewhat counterproductive.

BridgeAR commented Sep 19, 2017

Copy link
Copy Markdown
Member

Ping @evanlucas

jasnell commented Sep 20, 2017

Copy link
Copy Markdown
Member

I think it's likely safe to dismiss @evanlucas' objection since this was changed to a deprecation as he requested. Evan is on vacation right now so may not be able to rereview soon

jasnell dismissed evanlucas’s stale review September 20, 2017 08:06

Was changed to a deprecation as requested

jasnell commented Sep 20, 2017

Copy link
Copy Markdown
Member

Do we allow an undocumented API to be runtime deprecated directly without documenting and deprecating first?

Yes, so long as deprecations.md is updated appropriately.

BridgeAR commented Sep 22, 2017

Copy link
Copy Markdown
Member

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

BridgeAR commented Sep 23, 2017

Copy link
Copy Markdown
Member

Landed in 9d7574e

BridgeAR closed this Sep 23, 2017
BridgeAR pushed a commit that referenced this pull request Sep 23, 2017
module: deprecate Module._debug
9d7574e 
The _debug of Module is undocumented and it useless here.

PR-URL: #13948
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
JacksonTian deleted the _debug branch September 23, 2017 05:05
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 23, 2017
module: deprecate Module._debug
87411d9 
The _debug of Module is undocumented and it useless here.

PR-URL: nodejs/node#13948
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
jasnell mentioned this pull request Oct 30, 2017
Merged
tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018
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

jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
BridgeAR BridgeAR approved these changes
+3 more reviewers
TimothyGu TimothyGu approved these changes
targos targos approved these changes
evanlucas evanlucas left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

deprecations Issues and PRs related to deprecations. module Issues and PRs related to the module subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL