readline: promote _getCursorPos to public api #30687

Js-Brecht · 2019-11-27T20:12:03Z

Aliases Interface._getCursorPos to Interface.getCursorPos, for consumption as a public API.

If so desired, I can replace _getCursorPos method calls with getCursorPos, and create an alias the other way around. Doing it like this was just the simplest way to accomplish exposing _getCursorPos.

refs: #30347

Checklist

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

bnoordhuis

Needs docs and a regression test and I think I have a preference for simply renaming instead of aliasing it but perhaps other maintainers feel differently.

addaleax

Making this public sounds good to me, but like Ben said having a test + docs is necessary here.

jasnell · 2019-12-01T19:46:34Z

+1 to making this public as an alias with docs and tests added.

targos · 2019-12-01T20:06:55Z

can we invert the definitions so the name of the function doesn't have an underscore?

Js-Brecht · 2019-12-01T20:17:24Z

I should have some time tomorrow to get the docs & tests done. Had already planned on writing docs for it, but as a separate PR; it does make sense to just include it in this one, though.

I can also strip the underscore (and invert the alias, for backwards compatibility) since that seems to be the most desired? I see two votes for that, so far. Or maybe wait a couple more days and let more people weigh in 🤷‍♂️

Js-Brecht · 2019-12-02T19:43:35Z

I am looking into getting these tests done, but I need a bit of guidance.

_getCursorPos() is pretty well covered by the unit tests, and regression tests were what was requested. That tells me that you want to ensure that the new alias is covered by tests to ensure it incurs no breaking changes, but since the alias is running all of the same code, I'm a little bit at a loss.

Are you expecting a coverage test report of some kind for readline, or a unit test using the alias, or both?

Unit testing
1. I could write a simple unit test using the alias; that's easy enough
2. How much coverage is expected with alias unit test(s)?
  - Would successful function execution be enough for an alias?
  - If all of the existing functionality surrounding _getCursorPos() should be covered, then is there a recommended method for reusing the existing tests, but with the alias swapped in?
Coverage
1. I'm not sure what the recommended method would be for including any kind of coverage (attach to this PR, etc... ?)

Please advise.

bnoordhuis · 2019-12-03T15:26:02Z

You're right, test/parallel/test-readline-interface.js exercises _getCursorPos(). It's enough to update that to use the non-underscored name.

Js-Brecht · 2019-12-03T17:21:56Z

So basically use the alias in the tests? I had considered that, but then disregarded.

I will go ahead and strip the underscore in readline.js as requested (and invert the alias) today. Shouldn't even need to touch the tests in that case, yeah? They will all be using the alias to access the primary function.

Just committed the doc. Please let me know if it is not clear enough.

nodejs-github-bot · 2019-12-07T22:03:20Z

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

bnoordhuis

Can you update test/parallel/test-readline-interface.js to use the public API? Thanks.

Js-Brecht · 2019-12-09T00:44:20Z

@bnoordhuis done. Sorry about that

bnoordhuis

No problem. LGTM, thanks. There's a small conflict in readline.md but it should be trivial to resolve when rebasing.

Aliases Interface._getCursorPos to Interface.getCursorPos, for consumption as a public API. refs: nodejs#30347

Alias _getCursorPos() = getCursorPos() for backwards compatibility.

Js-Brecht · 2019-12-09T18:43:25Z

Merged updates in readline.md

Js-Brecht · 2019-12-09T18:47:22Z

One question: since this is going public now, should the YAML doc be "added: REPLACEME", or should I make it the version where the internal function was created? Looks like v0.7.6

doc/api/readline.md

nodejs-github-bot · 2019-12-10T00:23:56Z

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

Co-Authored-By: Ruben Bridgewater <ruben@bridgewater.de>

BridgeAR · 2019-12-10T00:24:12Z

@Js-Brecht please keep the REPLACEME as it is. It will be replaced with the Node.js version where this is first released in.

nodejs-github-bot · 2019-12-10T13:44:46Z

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

nodejs-github-bot · 2019-12-11T09:39:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27572/ ✔️

targos · 2019-12-16T10:22:48Z

Landed in a68729c

Alias _getCursorPos() = getCursorPos() for backwards compatibility. Refs: #30347 PR-URL: #30687 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Notable Changes: * cli: * add --trace-exit cli option (legendecas) #30516 * http,https: * increase server headers timeout (Tim Costa) #30071 * readline: * update ansi-regex (Ruben Bridgewater) #30907 * promote \_getCursorPos to public api (Jeremy Albright) #30687 * repl: * add completion preview (Ruben Bridgewater) #30907 * util: * add Set and map size to inspect output (Ruben Bridgewater) #30225 * wasi: * require CLI flag to require() wasi module (Colin Ihrig) #30963 PR-URL: #31010

Js-Brecht · 2019-12-24T20:10:04Z

Considering versions < v13 are still widely used, is it possible to merge this into v12 & v10 as well? If so, would that mean that I should submit a PR against the v10.x and v12.x branches?

MylesBorins · 2020-01-03T18:19:27Z

@Js-Brecht I've added an lts-watch label to this PR and we'll discuss in an upcoming LTS meeting when planning the next Semver-Minor for 12.x and 10.x

It appears like this lands cleanly on 12.x with minor conflicts in the docs. As for 10.x there are more conflicts... so we may have to wait a little bit to see how feasible this change is on that release line.

Js-Brecht · 2020-01-03T19:01:58Z

@MylesBorins Okay, sounds good. Please let me know if you'd like me to submit a new PR for 10.x. I haven't reviewed the 10.x code yet, but I imagine that these changes would be simple to implement there, too. All they really required was find/replace, and the addition of one line. Implementing it directly in the 10.x branch should resolve any conflicts, I would think.

Alias _getCursorPos() = getCursorPos() for backwards compatibility. Refs: #30347 PR-URL: #30687 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Nov 27, 2019

Js-Brecht marked this pull request as ready for review November 27, 2019 20:12

Js-Brecht force-pushed the js-brecht/readline-get-cursor-pos branch from a7b3b6f to c6d228a Compare November 27, 2019 21:14

Js-Brecht mentioned this pull request Nov 27, 2019

Expose readline.Interface "line", "cursor", "getCursorPos" #30347

Closed

bnoordhuis reviewed Nov 28, 2019

View reviewed changes

addaleax reviewed Nov 30, 2019

View reviewed changes

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 30, 2019

jasnell approved these changes Dec 3, 2019

View reviewed changes

lpinca approved these changes Dec 6, 2019

View reviewed changes

addaleax approved these changes Dec 7, 2019

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 7, 2019

bnoordhuis reviewed Dec 8, 2019

View reviewed changes

bnoordhuis approved these changes Dec 9, 2019

View reviewed changes

Js-Brecht added 4 commits December 9, 2019 08:40

readline: promote _getCursorPos to public api

5f4f780

Aliases Interface._getCursorPos to Interface.getCursorPos, for consumption as a public API. refs: nodejs#30347

doc: readline.getCursorPos() documentation

417dac1

readline: strip underscore from getCursorPos()

98f47ac

Alias _getCursorPos() = getCursorPos() for backwards compatibility.

tests: change _getCursorPos() to getCursorPos()

6b09369

Js-Brecht force-pushed the js-brecht/readline-get-cursor-pos branch from b2a800c to 6b09369 Compare December 9, 2019 18:42

BridgeAR approved these changes Dec 10, 2019

View reviewed changes

doc/api/readline.md Outdated Show resolved Hide resolved

YAML formatting

c784b7a

Co-Authored-By: Ruben Bridgewater <ruben@bridgewater.de>

targos closed this Dec 16, 2019

MylesBorins mentioned this pull request Dec 17, 2019

v13.5.0 proposal #31010

Merged

MylesBorins added lts-watch-v10.x labels Jan 3, 2020

targos mentioned this pull request Jan 15, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

BethGriggs added backport-requested-v10.x and removed lts-watch-v12.x labels Feb 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

readline: promote _getCursorPos to public api #30687

readline: promote _getCursorPos to public api #30687

Js-Brecht commented Nov 27, 2019

bnoordhuis left a comment

addaleax left a comment

jasnell commented Dec 1, 2019

targos commented Dec 1, 2019

Js-Brecht commented Dec 1, 2019

Js-Brecht commented Dec 2, 2019

bnoordhuis commented Dec 3, 2019

Js-Brecht commented Dec 3, 2019

nodejs-github-bot commented Dec 7, 2019

bnoordhuis left a comment

Js-Brecht commented Dec 9, 2019

bnoordhuis left a comment

Js-Brecht commented Dec 9, 2019

Js-Brecht commented Dec 9, 2019 •

edited

nodejs-github-bot commented Dec 10, 2019

BridgeAR commented Dec 10, 2019

nodejs-github-bot commented Dec 10, 2019

nodejs-github-bot commented Dec 11, 2019 •

edited by BridgeAR

targos commented Dec 16, 2019

Js-Brecht commented Dec 24, 2019

MylesBorins commented Jan 3, 2020

Js-Brecht commented Jan 3, 2020

readline: promote _getCursorPos to public api #30687

readline: promote _getCursorPos to public api #30687

Conversation

Js-Brecht commented Nov 27, 2019

Checklist

bnoordhuis left a comment

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

jasnell commented Dec 1, 2019

targos commented Dec 1, 2019

Js-Brecht commented Dec 1, 2019

Js-Brecht commented Dec 2, 2019

bnoordhuis commented Dec 3, 2019

Js-Brecht commented Dec 3, 2019

nodejs-github-bot commented Dec 7, 2019

bnoordhuis left a comment

Choose a reason for hiding this comment

Js-Brecht commented Dec 9, 2019

bnoordhuis left a comment

Choose a reason for hiding this comment

Js-Brecht commented Dec 9, 2019

Js-Brecht commented Dec 9, 2019 • edited

nodejs-github-bot commented Dec 10, 2019

BridgeAR commented Dec 10, 2019

nodejs-github-bot commented Dec 10, 2019

nodejs-github-bot commented Dec 11, 2019 • edited by BridgeAR

targos commented Dec 16, 2019

Js-Brecht commented Dec 24, 2019

MylesBorins commented Jan 3, 2020

Js-Brecht commented Jan 3, 2020

Js-Brecht commented Dec 9, 2019 •

edited

nodejs-github-bot commented Dec 11, 2019 •

edited by BridgeAR