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

module: deprecate Module._debug #13948

Closed
wants to merge 1 commit into from
Closed

module: deprecate Module._debug #13948

wants to merge 1 commit into from

Conversation

JacksonTian
Copy link
Contributor

@JacksonTian JacksonTian commented Jun 27, 2017 •
edited
Loading

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 nodejs-github-bot added the module Issues and PRs related to the module subsystem. label Jun 27, 2017
@jasnell
Copy link
Member

jasnell commented Jun 27, 2017

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 jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 27, 2017
@jasnell
Copy link
Member

jasnell commented Jun 27, 2017

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

@BridgeAR
Copy link
Member

This needs some more reviews from @nodejs/ctc

@BridgeAR
Copy link
Member

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

@jasnell
Copy link
Member

jasnell commented Aug 30, 2017

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
Copy link
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
evanlucas previously requested changes Aug 30, 2017
View reviewed changes
Copy link
Contributor

@evanlucas evanlucas left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

mscdex commented Aug 30, 2017

It wouldn't hurt to deprecate it.

@jasnell
Copy link
Member

jasnell commented Aug 31, 2017

Yep, I'm good with a regular deprecation.

@JacksonTian
Copy link
Contributor Author

Thanks all. I will change to deprecate it first.

@JacksonTian JacksonTian changed the title module: remove the intermediate Module._debug module: deprecate Module._debug Sep 2, 2017
@JacksonTian
Copy link
Contributor Author

Hi everyone, I updated this PR with deprecation.

TimothyGu
TimothyGu approved these changes Sep 2, 2017
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

Lgtm besides the nits.

doc/api/deprecations.md Outdated
@@ -660,6 +660,14 @@ Type: Runtime

`REPLServer.parseREPLKeyword()` was removed from userland visibility.

<a id="DEP0076"></a>
### DEP0075: Module._debug()
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind escaping the underscore?

Copy link
Member

Choose a reason for hiding this comment

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

Also the DEP ID is off.

jasnell
jasnell reviewed Sep 5, 2017
View reviewed changes
doc/api/deprecations.md Outdated
@@ -660,6 +660,14 @@ Type: Runtime

`REPLServer.parseREPLKeyword()` was removed from userland visibility.

<a id="DEP0076"></a>
### DEP0076: Module.\_debug()
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And the

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

should be DEP00XX also?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, all of the DEP00.. identifiers.

@JacksonTian
module: deprecate Module._debug
a381a7f 
The _debug of Module is undocumented and it useless here.
@JacksonTian
Copy link
Contributor Author

Hi all, I have updated this PR.

@BridgeAR
Copy link
Member

@evanlucas the PR was updated with your suggestion. PTAL

@thefourtheye
Copy link
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
Copy link
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
Copy link
Member

Ping @evanlucas

@jasnell
Copy link
Member

jasnell commented Sep 20, 2017

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 jasnell dismissed evanlucas’s stale review September 20, 2017 08:06

Was changed to a deprecation as requested

@jasnell
Copy link
Member

jasnell commented Sep 20, 2017

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
Copy link
Member

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

@BridgeAR
Copy link
Member

Landed in 9d7574e

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

PR-URL: #13948
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@JacksonTian 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
@JacksonTian @addaleax
module: deprecate Module._debug
87411d9 
The _debug of Module is undocumented and it useless here.

PR-URL: nodejs/node#13948
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
@tniessen tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@jasnell jasnell jasnell approved these changes

@evanlucas evanlucas evanlucas left review comments

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.