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.

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

readline: promote _getCursorPos to public api #30687

Closed

Conversation

@Js-Brecht
Copy link
Contributor

Js-Brecht commented Nov 27, 2019

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
@Js-Brecht Js-Brecht marked this pull request as ready for review Nov 27, 2019
@Js-Brecht Js-Brecht force-pushed the Js-Brecht:js-brecht/readline-get-cursor-pos branch from a7b3b6f to c6d228a Nov 27, 2019
Copy link
Member

bnoordhuis left a comment

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.

Copy link
Member

addaleax left a comment

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

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Dec 1, 2019

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

@targos

This comment has been minimized.

Copy link
Member

targos commented Dec 1, 2019

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

@Js-Brecht

This comment has been minimized.

Copy link
Contributor Author

Js-Brecht commented Dec 1, 2019

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

This comment has been minimized.

Copy link
Contributor Author

Js-Brecht commented Dec 2, 2019

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?

  1. 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?
  2. 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

This comment has been minimized.

Copy link
Member

bnoordhuis commented Dec 3, 2019

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

This comment has been minimized.

Copy link
Contributor Author

Js-Brecht commented Dec 3, 2019

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.

@jasnell
jasnell approved these changes Dec 3, 2019
@lpinca
lpinca approved these changes Dec 6, 2019
@nodejs-github-bot

This comment has been minimized.

Copy link
Member

bnoordhuis left a comment

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

@Js-Brecht

This comment has been minimized.

Copy link
Contributor Author

Js-Brecht commented Dec 9, 2019

@bnoordhuis done. Sorry about that

Copy link
Member

bnoordhuis left a comment

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

Js-Brecht added 4 commits Nov 27, 2019
Aliases Interface._getCursorPos to Interface.getCursorPos,
for consumption as a public API.

refs: #30347
Alias _getCursorPos() = getCursorPos() for backwards
compatibility.
@Js-Brecht Js-Brecht force-pushed the Js-Brecht:js-brecht/readline-get-cursor-pos branch from b2a800c to 6b09369 Dec 9, 2019
@Js-Brecht

This comment has been minimized.

Copy link
Contributor Author

Js-Brecht commented Dec 9, 2019

Merged updates in readline.md

@Js-Brecht

This comment has been minimized.

Copy link
Contributor Author

Js-Brecht commented Dec 9, 2019

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 Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 10, 2019

@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

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

Copy link

nodejs-github-bot commented Dec 11, 2019

@targos

This comment has been minimized.

Copy link
Member

targos commented Dec 16, 2019

Landed in a68729c

@targos targos closed this Dec 16, 2019
targos added a commit that referenced this pull request Dec 16, 2019
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>
MylesBorins added a commit that referenced this pull request Dec 17, 2019
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>
@MylesBorins MylesBorins mentioned this pull request Dec 17, 2019
MylesBorins added a commit that referenced this pull request Dec 18, 2019
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
MylesBorins added a commit that referenced this pull request Dec 18, 2019
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

This comment has been minimized.

Copy link
Contributor Author

Js-Brecht commented Dec 24, 2019

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

This comment has been minimized.

Copy link
Member

MylesBorins commented Jan 3, 2020

@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

This comment has been minimized.

Copy link
Contributor Author

Js-Brecht commented Jan 3, 2020

@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.

targos added a commit that referenced this pull request Jan 14, 2020
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>
targos added a commit that referenced this pull request Jan 14, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.