repl: deprecate REPLServer.prototype.turnOffEditorMode by lance · Pull Request #15136 · nodejs/node

lance · 2017-09-01T16:59:35Z

This commit ~~exposes REPLServer.prototype.turnOnEditorMode() which handles the necessary internal changes to enter editor mode~~ deprecates REPLServer.prototype.turnOffEditorMode() in the REPL, ~~instead of having them scattered about~~ and consolidates some duplicated code for turning on editor mode as a non-public function.

Checklist

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

Affected core subsystem(s)

repl, doc

benjamingr · 2017-09-01T17:11:00Z

Why not make it a function instead of a method if the goal is code duplication?

function turnOnEditorMode(repl) {
  repl.editorMode = true;
  REPLServer.super_.prototype.setPrompt.call(repl, '');
}

lance · 2017-09-01T17:20:35Z

@benjamingr this was my original intent, but then I noticed that turnOffEditorMode already existed on the prototype, so I decided on consistency. TBH, I'd prefer to make both of these just plain old functions and hide them from user land. But now that turnOffEditorMode has been in the wild, that's a bit of a pain.

benjamingr · 2017-09-01T17:50:34Z

Pinging @nodejs/ctc regarding an API addition in a stable module.

BridgeAR · 2017-09-01T17:55:07Z

I would definitely prefer this to be a private function. I think a good way to handle it would be to deprecate the current R.p.turnOffEditorMode(). A bit cleaning up might be a good idea.

lance · 2017-09-01T17:58:39Z

OK, it seems there is consensus around keeping this private and not adding to the API. I will modify this PR to make turnOnEditorMode a private function, and deprecate REPLServer.prototype.turnOffEditorMode with the eventual goal to have that as a private function as well.

Refs: #7619

refack · 2017-09-01T21:25:44Z

OK, it seems there is consensus around keeping this private and not adding to the API.

I'm not 100% sold on deprecating this this API. editorMode is a nice feature of the REPL, maybe implementors should have a way to turn it on and off.

/cc @Fishrock123

P.S. I could not find any uses of turnOffEditorMode in either Gzemnid or GitHub search.

lance · 2017-09-04T20:53:51Z

@princejwesley I personally prefer a calling syntax like

function _turnOnEditorMode(repl) {
  // do something with repl
}

So that's how I wrote the function. But that doesn't work as far as I know, with util.deprecate(). When using the deprecate() function on an object's method, you use this to access the caller. So, the internal implementation uses that calling style, e.g.

function _turnOffEditorMode() {
  // `this` is the calling context
}

I could modify _turnOnEditorMode() to be consistent, but tbh I'd rather modify _turnOffEditorMode() to not use this once REPLServer.prototype.turnOffEditorMode() is fully deprecated and gone.

Either way works for me.

lance · 2017-09-04T20:58:15Z

@refack REPLServer.prototype.turnOffEditorMode() was only added a year ago, and without documentation, so it's understandable that there is not wide (or any known) usage of it. Plus, it wasn't very useful as it was without a corresponding REPLServer.prototype.turnOnEditorMode() which does not exist. Unless we really want to add a new feature, I'd lean towards eliminating public API rather than adding one of perhaps questionable usefulness.

lance · 2017-09-07T20:35:02Z

@BridgeAR @princejwesley @refack ping.

I hope I have answered your questions. And if not, let me know what specific actions you think I should take.

@benjamingr as it is now, with my recent changes, this does not add any API to the REPLServer. I'll update the PR description to reflect that. It does deprecate a currently unpublished method on REPLServer.prototype, so I'm not sure what this means with regard to @nodejs/tsc involvement/approval.

BridgeAR

In general this is the right approach out of my perspective. Having two different kinds of function calls is a bit ugly though and the deprecation must be documented.

BridgeAR · 2017-09-07T20:44:20Z

lib/repl.js

I think it would be nicer to have _turnOffEditorMode(this); (or self) instead of _turnOffEditorMode.call(this);
It is weird that turnOn and turnOff works differently.

In that case the prototype function would have to be changed to look somewhat like

function () { _turnOffEditorMode(this); }

BridgeAR · 2017-09-07T20:45:25Z

lib/repl.js

The deprecation must be documented as well in the deprecations.md.

Yes and deprecations are semver-major.

lance · 2017-09-08T16:33:20Z

Code updated and deprecation documentation added.
CI: https://ci.nodejs.org/job/node-test-pull-request/10003/

Edit: Single CI failure on smartos seems to be unrelated

not ok 117 parallel/test-benchmark-buffer
  ---
  duration_ms: 120.524
  severity: fail
  stack: |-
    timeout
  ...

BridgeAR

LGTM % comment

BridgeAR · 2017-09-11T04:09:24Z

doc/api/repl.md

Actually I think we do not have to document this at all. This would create bigger confusion than providing benefit.

Hmm - I would agree, but previous similar deprecations have been handled in this way.

See:

repl: Undocumented public methods on REPLServer.prototype #7619 (comment)

https://github.com/nodejs/node/blob/master/doc/api/deprecations.md#dep0074-replserverbufferedcommand.

I think documenting this is the opposite of what we actually want to achieve. I do not really read in that comment that we necessarily have to document this either and I would rather only have the deprecation notice that this was never meant to be exposed and that it is now deprecated but no further documentation.

@jasnell what do you think about this?

By the way - the deprecation docs do not cover this part at all. Therefore I think it is safe to remove this.

@BridgeAR are you suggesting removing this commit entirely? Or just removing the wording from repl.md?

Only the part from the repl.md.

BridgeAR · 2017-09-20T05:25:16Z

This needs some LGs due to being semver-major @nodejs/tsc

BridgeAR · 2017-09-22T20:59:40Z

Ping @nodejs/tsc PTAL

BridgeAR · 2017-09-27T21:29:45Z

@lance would you be so kind and rebase?

This commit exposes `REPLServer.prototype.turnOnEditorMode()` which handles the necessary internal changes required instead of having them scattered about.

This deprecates the current REPLServer.prototype.turnOffEditorMode

Also, address some concerns from code review.

lance · 2017-09-27T21:39:30Z

@BridgeAR kindness applied! :)

BridgeAR · 2017-09-27T22:45:08Z

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

lance · 2017-09-28T01:00:01Z

Not happy with the results of that CI. I scanned a couple of the failures just briefly, and they seem unrelated. But it's surprising there are so many red dots. Trying again.

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

refack · 2017-09-28T02:14:10Z

@lance CI is clean, failures are unrelated (even linter)

This deprecates the current REPLServer.prototype.turnOffEditorMode and adds a private function for turnOffEditorMode which handles the necessary internal changes required instead of having them scattered about. PR-URL: #15136 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR · 2017-09-28T04:56:11Z

Landed in e416b3e

I fixed the merge conflicts while landing.

This slipped through while landing. Ref: nodejs#15136

This deprecates the current REPLServer.prototype.turnOffEditorMode and adds a private function for turnOffEditorMode which handles the necessary internal changes required instead of having them scattered about. PR-URL: nodejs/node#15136 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

This slipped through while landing. PR-URL: #15668 Refs: #15136 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

This slipped through while landing. PR-URL: nodejs/node#15668 Refs: nodejs/node#15136 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

lance requested review from BridgeAR and princejwesley September 1, 2017 16:59

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

lance added doc Issues and PRs related to the documentations. question Issues that look for answers. labels Sep 1, 2017

mscdex removed doc Issues and PRs related to the documentations. question Issues that look for answers. labels Sep 1, 2017

BridgeAR requested changes Sep 7, 2017

View reviewed changes

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

lance changed the title ~~repl: eliminate code duplication for editorMode~~ repl: deprecate REPLServer.prototype.turnOffEditorMode Sep 8, 2017

BridgeAR approved these changes Sep 11, 2017

View reviewed changes

princejwesley approved these changes Sep 11, 2017

View reviewed changes

benjamingr approved these changes Sep 15, 2017

View reviewed changes

BridgeAR approved these changes Sep 20, 2017

View reviewed changes

BridgeAR requested a review from a team September 22, 2017 20:59

BridgeAR added tsc-review and removed tsc-review labels Sep 22, 2017

addaleax approved these changes Sep 22, 2017

View reviewed changes

BridgeAR requested a review from a team September 24, 2017 15:29

jasnell approved these changes Sep 25, 2017

View reviewed changes

lance added 4 commits September 27, 2017 17:37

repl: eliminate code duplication for editorMode

d203d15

This commit exposes `REPLServer.prototype.turnOnEditorMode()` which handles the necessary internal changes required instead of having them scattered about.

repl: make turnO[n|ff]EditorMode private functions

fd4ffb8

This deprecates the current REPLServer.prototype.turnOffEditorMode

repl: document REPLServer.turnOffEditorMode deprecation

12a1d45

Also, address some concerns from code review.

repl: remove deprecation notice for turnOffEditorMode

48e1644

BridgeAR closed this Sep 28, 2017

BridgeAR added a commit to BridgeAR/node that referenced this pull request Sep 28, 2017

repl: fix deprecation code

bea2d58

This slipped through while landing. Ref: nodejs#15136

BridgeAR mentioned this pull request Sep 28, 2017

repl: fix deprecation code #15668

Closed

4 tasks

lance mentioned this pull request Oct 16, 2017

repl: Undocumented public methods on REPLServer.prototype #7619

Closed

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

Uh oh!

Conversation

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

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

benjamingr commented Sep 1, 2017

Uh oh!

lance commented Sep 1, 2017

Uh oh!

benjamingr commented Sep 1, 2017

Uh oh!

BridgeAR commented Sep 1, 2017

Uh oh!

lance commented Sep 1, 2017

Uh oh!

refack commented Sep 1, 2017

Uh oh!

lance commented Sep 4, 2017

Uh oh!

lance commented Sep 4, 2017

Uh oh!

lance commented Sep 7, 2017

Uh oh!

BridgeAR left a comment

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 commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR left a comment

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!

BridgeAR Sep 13, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 20, 2017

Uh oh!

BridgeAR commented Sep 22, 2017

Uh oh!

BridgeAR commented Sep 27, 2017

Uh oh!

lance commented Sep 27, 2017

Uh oh!

BridgeAR commented Sep 27, 2017

Uh oh!

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

Uh oh!

Uh oh!

refack commented Sep 28, 2017

Uh oh!

BridgeAR commented Sep 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

lance commented Sep 1, 2017 •

edited

Loading

lance commented Sep 8, 2017 •

edited

Loading

BridgeAR Sep 13, 2017 •

edited

Loading

lance commented Sep 28, 2017 •

edited

Loading