repl: deprecate REPLServer.parseREPLKeyword by lance · Pull Request #14223 · nodejs/node

lance · 2017-07-13T18:30:58Z

This method does not need to be visible to user code. It has been
undocumented since it was introduced which was perhaps v0.8.9, as
far as I can tell.

The motivation for this change is that the method is simply an
implementation detail of the REPLServer behavior, and does
not need to be exposed to user code.

This change adds documentation of the method with a deprecation
warning as recommended by @jasnell in
#7619 (comment).

Refs: #7619

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

repl, doc

refack

You should add a test that using this function raises the deprecation warning
Ref:

node/test/parallel/test-fs-makeStatsCallback.js

Line 16 in 7c12d12

common.expectWarning('DeprecationWarning', warn);

lance · 2017-07-14T19:13:51Z

@refack test added

refack · 2017-07-14T22:42:24Z

doc/api/repl.md

Nit: (can be done while landing) XXXX should be REPLACEME

refack · 2017-07-14T22:45:06Z

FWIW: https://ci.nodejs.org/job/node-test-commit/11116/
(also ticked the tests and/or benchmarks are included box)

refack · 2017-07-14T23:40:28Z

lint:

not ok 2 - /usr/home/iojs/build/workspace/node-test-linter/test/parallel/test-repl-deprecations.js
  ---
  message: Too many blank lines at the end of file. Max of 0 allowed.
  severity: error
  data:
    line: 22
    column: 1
    ruleId: no-multiple-empty-lines
  ...

lance · 2017-07-16T15:52:24Z

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

jasnell · 2017-07-17T15:54:56Z

lib/repl.js

Perhaps rather than passing repl in as an argument, restructure this to assume this === repl ... such that in the util.deprecate call above you can do:

util.deprecate(_parseREPLKeyword, '...', '...');

Wouldn't that just introduce another property on the class?

@lance You can do .call().

@TimothyGu sure, but how is that any better than what is there now? Wouldn't that mean doing something like this?

REPLServer.prototype.parseREPLKeyword = util.deprecate( function(keyword, rest) { return _parseREPLKeyword.call(this, keyword, rest); }, 'REPLServer.parseREPLKeyword() is deprecated', 'DEP0XX');

I don't see how this improves the code. Is .call() inherently faster?

No, in that case you can just

REPLServer.prototype.parseREPLKeyword = util.deprecate(_parseREPLKeyword, 'REPLServer.parseREPLKeyword() is deprecated', 'DEP0XX');

Or am I missing something?

jasnell · 2017-07-17T15:56:36Z

test/parallel/test-repl-deprecations.js

There should be only one call to common.expectWarning(). The deprecation warning is only going to be emitted once. What calling common.expectWarning() twice does is set up two identical listeners for the process.on('warning') event that is only emitted once.

I have a local change with only a single call to common.expectWarning(). But what I find confusing is that the tests pass whether there is one or two calls to this. Does common.expectWarning() not fail if the warning isn't issued?

lance · 2017-07-18T18:54:13Z

@jasnell made changes per your request. PTAL.

lance · 2017-07-27T13:48:28Z

Ping @jasnell - it needs a rebase on deprecations.md but otherwise, just following up here...

jasnell · 2017-07-31T19:15:58Z

test/parallel/test-repl-deprecations.js

Why separate these out into separate functions like this?

Only the assumption that there may be future deprecations tests, and the test() function would run them all. I have no particular affinity for this approach though.

jasnell

In general, this looks ok, but the test could use a bit of reworking. LGTM overall tho.

@jasnell

This method does not need to be visible to user code. It has been undocumented since it was introduced which was perhaps v0.8.9, as far as I can tell. This change is as recommended by @jasnell in #7619 (comment). This change is only for `parseREPLKeyword()`.

lance · 2017-08-02T18:43:32Z

Landed in 766506a

This method does not need to be visible to user code. It has been undocumented since it was introduced which was perhaps v0.8.9. The motivation for this change is that the method is simply an implementation detail of the REPLServer behavior, and does not need to be exposed to user code. This change adds documentation of the method with a deprecation warning, and a test that the method is actually documented. PR-RUL: #14223 Refs: #7619 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax · 2017-08-07T13:12:43Z

Shouldn’t this have been semver-major? I’m labelling as such but I’d like to point out that this should really have gotten another @nodejs/ctc approval according to our rules.

addaleax · 2017-08-10T16:18:07Z

(@nodejs/release: This is semver-major and has no valid PR-URL: metadata, any idea how to work around this showing up in branch-diff until the end of time?)

gibfahn · 2017-08-12T22:39:43Z

(@nodejs/release: This is semver-major and has no valid PR-URL: metadata, any idea how to work around this showing up in branch-diff until the end of time?)

I don't think there's a way at the moment, we have some of these for 6.x. Maybe teaching branch-diff to parse commit comments with valid metadata would be the way to go.

lance · 2017-08-23T16:28:31Z

@addaleax @gibfahn The PR-URL: metadata was a typo on my part (PR-RUL: #14223) in 766506a. My apologies for the error. WRT semver-major, if I had realized, I would have waited for a 3rd. Again, my mistake.

This method does not need to be visible to user code. It has been undocumented since it was introduced which was perhaps v0.8.9. The motivation for this change is that the method is simply an implementation detail of the REPLServer behavior, and does not need to be exposed to user code. This change adds documentation of the method with a deprecation warning, and a test that the method is actually documented. PR-RUL: #14223 Refs: #7619 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Jul 13, 2017

refack suggested changes Jul 13, 2017

View reviewed changes

refack reviewed Jul 14, 2017

View reviewed changes

doc/api/repl.md Outdated

Copy link

Contributor

refack Jul 14, 2017

Choose a reason for hiding this comment

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

Nit: (can be done while landing) XXXX should be REPLACEME

refack approved these changes Jul 14, 2017

View reviewed changes

refack approved these changes Jul 15, 2017

View reviewed changes

jasnell reviewed Jul 17, 2017

View reviewed changes

jasnell reviewed Jul 31, 2017

View reviewed changes

jasnell approved these changes Jul 31, 2017

View reviewed changes

lance added 5 commits August 2, 2017 13:57

repl: add test for deprecated REPL properties

bf9e499

address linting and nits

5285d8a

repl: rework REPLServer.parseREPLKeyword

bff62e9

fix: address unnecessary indirection in repl test

3b2daff

lance closed this Aug 2, 2017

lance deleted the 7619-repl-keyword-deprecation branch August 2, 2017 18:43

addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 7, 2017

refack added dont-land-on-v8.x and removed dont-land-on-v8.x labels Aug 7, 2017

refack mentioned this pull request Aug 7, 2017

meta: handling of undocumented endpoint #14679

Closed

lance mentioned this pull request Oct 16, 2017

repl: Undocumented public methods on REPLServer.prototype #7619

Closed

gibfahn mentioned this pull request Oct 30, 2017

[v8.x backport] repl: remove internal frames from runtime errors #16457

Closed

3 tasks

gibfahn added dont-land-on-v8.x labels Oct 31, 2017

Uh oh!

Conversation

lance commented Jul 13, 2017 • edited by refack Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

lance commented Jul 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Jul 14, 2017

Uh oh!

lance commented Jul 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lance Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lance commented Jul 18, 2017

Uh oh!

lance commented Jul 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

lance commented Aug 2, 2017

Uh oh!

addaleax commented Aug 7, 2017

Uh oh!

addaleax commented Aug 10, 2017

Uh oh!

gibfahn commented Aug 12, 2017

Uh oh!

lance commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lance commented Jul 13, 2017 •

edited by refack

Loading

refack commented Jul 14, 2017 •

edited

Loading

lance Jul 18, 2017 •

edited

Loading

lance commented Aug 23, 2017 •

edited

Loading