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

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

@nodejs-github-bot nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Nov 27, 2019
@Js-Brecht Js-Brecht marked this pull request as ready for review November 27, 2019 20:12
@Js-Brecht Js-Brecht force-pushed the js-brecht/readline-get-cursor-pos branch from a7b3b6f to c6d228a Compare November 27, 2019 21:14
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

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 addaleax left a comment

Choose a reason for hiding this comment

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

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

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

jasnell commented Dec 1, 2019

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

@targos
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
Copy link
Contributor Author

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

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

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

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.

@addaleax 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
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

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

@Js-Brecht
Copy link
Contributor Author

@bnoordhuis done. Sorry about that

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

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 Js-Brecht force-pushed the js-brecht/readline-get-cursor-pos branch from b2a800c to 6b09369 Compare December 9, 2019 18:42
@Js-Brecht
Copy link
Contributor Author

Merged updates in readline.md

@Js-Brecht
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
Copy link
Collaborator

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

@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
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 11, 2019

@targos
Copy link
Member

targos commented Dec 16, 2019

Landed in a68729c

@targos targos closed this Dec 16, 2019
targos pushed 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 pushed 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
Copy link
Contributor Author

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

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

@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 pushed 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>
BethGriggs pushed a commit that referenced this pull request Feb 6, 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>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. readline Issues and PRs related to the built-in readline module. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants