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

doc: undocumented entities in code example in repl.md #12686

Closed
vsemozhetbyt opened this issue Apr 27, 2017 · 8 comments
Closed

doc: undocumented entities in code example in repl.md #12686

vsemozhetbyt opened this issue Apr 27, 2017 · 8 comments
Labels
doc Issues and PRs related to the documentations. repl Issues and PRs related to the REPL subsystem.

Comments

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 27, 2017

  • Subsystem: doc, repl

Currently, we have 3 undocumented entities in code example in repl.md:

replServer.lineParser.reset()
replServer.bufferedCommand
replServer.close()

The first one will be gone since Node.js v8.0.0

The second one can be considered as an acceptable ad-hoc internal revelation.

Is it worth to document the third one, replServer.close()?

@vsemozhetbyt vsemozhetbyt added doc Issues and PRs related to the documentations. repl Issues and PRs related to the REPL subsystem. labels Apr 27, 2017
@cjihrig
Copy link
Contributor

cjihrig commented Apr 27, 2017

The last two should probably be documented.

@anchnk
Copy link

anchnk commented May 4, 2017

@vsemozhetbyt If this hasn't been addressed yet I can take that responsibility.

Do you think the close() method and the bufferedCommand property should be added to the REPLServer class description or adding comments to code snippets is enough ?

@vsemozhetbyt
Copy link
Contributor Author

@anchnk I am not sure, sorry. cc @nodejs/documentation ?

If nobody else answers, feel free to open a PR with any decision and we may correct this later in the PR.

@lance
Copy link
Member

lance commented Jun 7, 2017

@cjihrig I am curious why REPLServer.bufferedCommand should be documented. Is this one of those undocumented properties that people tend to depend on? If not, I'm curious if it really makes sense to start documenting it now. It really does feel like an implementation detail. Somewhat related, is an issue I opened last year which I (embarrassingly) have done nothing about yet. #7619

@cjihrig
Copy link
Contributor

cjihrig commented Jun 7, 2017

@lance that's just my opinion because it is a public property without an underscore. If it's going to stick around like that, it should probably be documented. Otherwise, we should move to deprecate/remove it.

@lance
Copy link
Member

lance commented Jun 7, 2017

@cjihrig in my opinion, minimizing the public surface area is in the best long term interest, especially in cases like this where it does seem that the property is a leakage of the implementation. Making this property public and documented means that the implementation, or at least this part of it anyway, needs to remain in place in spite of potential needs/changes in the future.

I understand that removing something that's public, even if it's not documented, is generally frowned upon - or at least thought about pretty thoroughly. As you said, it's just my opinion. :)

@cjihrig
Copy link
Contributor

cjihrig commented Jun 7, 2017

I completely agree that less surface area is better. These things should have probably never been made public API. But they're still public, and get harder to remove all the time.

addaleax pushed a commit that referenced this issue Aug 1, 2017
The `REPLServer.bufferedCommand` property was undocumented, except
for its usage appearing, unexplained, in an example for
`REPLServer.defineCommand`. This commit deprecates that property,
privatizes it, and adds a `REPLServer.clearBufferedCommand()`
function that will clear the buffer.

PR-URL: #13687
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <mhdawson@ibm.com>
Reviewed-By: Ruben Bridgewater <ruben.bridgewater@fintura.de>

Refs: #12686
@lance
Copy link
Member

lance commented Aug 15, 2017

In #7619 @jasnell suggests close() should be considered documented and left as-is since it's inherited from readline.Interface.

Now that lineParser is gone as of 8.x and REPLServer.bufferedCommand has been made private, I think this can be closed.

@lance lance closed this as completed Aug 15, 2017
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. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants