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: deprecate undocumented exports #3862

Closed
wants to merge 0 commits into from

Conversation

Projects
None yet
6 participants
@cjihrig
Copy link
Contributor

commented Nov 16, 2015

This commit moves the readline's undocumented exports into an internal module. The existing public exports are given a deprecation notice.

Refs: #3847
Fixes: #3836

R= @jasnell

@romankl romankl added the readline label Nov 16, 2015

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 16, 2015

LGTM
Technically this would likely need to be semver-major.

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Nov 16, 2015

Looks like semver-major to me.

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2015

Yes, semver-major for sure

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Nov 16, 2015

Are four functions mentioned in #3836 the only exports removed in this commit?

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2015

The four functions from #3836, plus emitKeys(), which is already private and needed by emitKeypressEvents().

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Nov 16, 2015

Hm… I am not sure if those are unused. Will post greps later, probably tomorrow.
Maybe we should expose deprecated versions for a release.

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2015

There are deprecation messages in place for all of them.

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Nov 17, 2015

@cjihrig Ah, sorry, I missed that =), was too sleepy yesterday.
Should be good, then, I guess. I will do some greps later today.

@tejasmanohar

This comment has been minimized.

Copy link

commented Nov 17, 2015

👍

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2015

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 17, 2015

CI is red but failures look unrelated. Do we want to go ahead and land or give it a bit?

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Nov 17, 2015

@jasnell Give me some time, I will take a quick look at the usage of those methods.

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 17, 2015

@ChALkeR : +1

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 18, 2015

@nodejs/ctc ... a quick sanity check review on this would be appreciated

@mscdex

View changes

lib/readline.js Outdated
return str.replace(new RegExp(metaKeyCodeReAnywhere.source, 'g'), '');
}
exports.stripVTControlCharacters = stripVTControlCharacters;
exports.emitKeypressEvents = internalUtil.deprecate(emitKeypressEvents,

This comment has been minimized.

Copy link
@mscdex

mscdex Nov 18, 2015

Contributor

I think we should at least keep the style consistent here, preferably with each argument on a separate line (for better readability).

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2015

One minor nit, otherwise LGTM.

@ChALkeR

This comment has been minimized.

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Nov 18, 2015

Out of those, emitKeypressEvents seems used by several modules. Is there a reason why it should?

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 18, 2015

@ChALkeR ... I'm sorry, is there a reason why it should what?

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2015

Would there be value to leaving emitKeypressEvents() public and documenting it? the other functions seem to mostly be helpers, while emitKeypressEvents() may add real value.

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 18, 2015

@cjihrig ... that's definitely a good possibility. I'd be ok with that.

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Nov 18, 2015

@jasnell Sorry, English is not my native language.

I wanted to ask if there are any specific reasons why several modules have to call emitKeypressEvents, which was meant to be an internal method. If those modules have a valid reason for calling it, we could consider adding it to the API instead.

Note: I'm not familiar with repl code, guessing that just looking at the actual usage in modules.

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 18, 2015

@ChALkeR ... no worries :-) There may or may not be good reasons for modules calling it but given that they are and we made the mistake of not explicitly marking those as private using the _ prefix, we may as well just go ahead and document that one and keep it as @cjihrig suggests. The other ones I think we're safe deprecating and moving .

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Nov 18, 2015

@jasnell It's directly used by only 7 modules (plus the false negatives), so deprecating it shouldn't be such a problem. The question is — is that method a valuable addition the the API that we could want to keep?

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2015

The ability to emit keypress events seems useful to me.

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2015

Undid changes regarding emitKeypressEvents(). New CI: https://ci.nodejs.org/job/node-test-pull-request/791/

cjihrig added a commit that referenced this pull request Nov 19, 2015

readline: deprecate undocumented exports
This commit moves several of readline's undocumented functions
into an internal module. Specifically, isFullWidthCodePoint,
stripVTControlCharacters, getStringWidth, and emitKeys are
moved to the internal module. The existing public exports
of the first three functions are given a deprecation notice.

Refs: #3847
Fixes: #3836
PR-URL: #3862
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>

@cjihrig cjihrig closed this Nov 19, 2015

@cjihrig cjihrig force-pushed the cjihrig:rl branch to ca2e8b2 Nov 19, 2015

@cjihrig cjihrig deleted the cjihrig:rl branch Nov 19, 2015

@jasnell jasnell referenced this pull request Mar 17, 2016

Closed

Planning for v6 #5766

@addaleax addaleax referenced this pull request Apr 27, 2016

Merged

readline: remove deprecated methods #6423

2 of 2 tasks complete

cjihrig added a commit to cjihrig/node-1 that referenced this pull request May 2, 2016

readline: remove deprecated methods
This commit removes the deprecated exports getStringWidth(),
stripVTControlCharacters(), and isFullWidthCodePoint(). It also
removes codePointAt() in its entirety, as it was deprecated and
not being used by core.

Refs: nodejs#3862
PR-URL: nodejs#6423
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

joelostrowski added a commit to joelostrowski/node that referenced this pull request May 4, 2016

readline: remove deprecated methods
This commit removes the deprecated exports getStringWidth(),
stripVTControlCharacters(), and isFullWidthCodePoint(). It also
removes codePointAt() in its entirety, as it was deprecated and
not being used by core.

Refs: nodejs#3862
PR-URL: nodejs#6423
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
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.