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

Add in missing docs for exposed readline functions #3847

Closed
wants to merge 1 commit into from

Conversation

tejasmanohar
Copy link

No description provided.

@tejasmanohar tejasmanohar changed the title Readline docs Add in missing docs for exposed readline functions Nov 16, 2015
@thefourtheye thefourtheye added doc Issues and PRs related to the documentations. readline Issues and PRs related to the built-in readline module. labels Nov 16, 2015
@JungMinu
Copy link
Member

@tejasmanohar would you please describe the commit log in detail?
Here's the guideline:

Writing good commit logs is important. A commit log should describe what changed and why. Follow these guidelines when writing one:

The first line should be 50 characters or less and contain a short description of the change prefixed with the name of the changed subsystem (e.g. "net: add localAddress and localPort to Socket").
Keep the second line blank.
Wrap all other lines at 72 columns.
A good commit log can look something like this:

subsystem: explaining the commit in one line

Body of commit message is a few lines of text, explaining things
in more detail, possibly giving some background about the issue
being fixed, etc. etc.

The body of the commit message can be several paragraphs, and
please do proper word-wrap and keep columns shorter than about
72 characters or so. That way `git log` will show things
nicely even when it is indented.
The header line should be meaningful; it is what other people see when they run git shortlog or git log --oneline.

@@ -308,6 +308,24 @@ a `"resize"` event on the `output` if/when the columns ever change

Move cursor to the specified position in a given TTY stream.

## readline.emitKeypressEvents(stream)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is meant to be public or not

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not.

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

@nodejs/ctc

@Fishrock123
Copy link
Contributor

-1, These should really have been prefixed with an underscore since I am quite sure they are meant for internal use.

## readline.stripVTControlCharacters(str)

Tries to remove all VT control characters. Use to estimate displayed string
width. **May be buggy due to not running a real state machine.**
Copy link
Contributor

Choose a reason for hiding this comment

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

i.e. not public XD

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

@Fishrock123 ... should we have a PR that prefixes them to underscored versions and deprecates the originals?

@cjihrig
Copy link
Contributor

cjihrig commented Nov 16, 2015

Also -1 to documenting private features.

@tejasmanohar
Copy link
Author

Hm, why are they exported if they're private?

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

@tejasmanohar ... as @Fishrock123 indicates, it's more an old naming error than anything else. These methods should have been prefixed with _ but they weren't. The proper action now (as I suggest) is to rename them, create aliases, then deprecate those aliases.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 16, 2015

@jasnell I'll take care of it. Is it just these 4 methods that need it?

@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

@cjihrig I believe so.

@jasnell
Copy link
Member

jasnell commented Nov 17, 2015

Closing in favor of #3862

@jasnell jasnell closed this Nov 17, 2015
cjihrig added a commit that referenced this pull request Nov 19, 2015
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. readline Issues and PRs related to the built-in readline module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants