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: repl: add defineComand and displayPrompt #3765

Closed
wants to merge 1 commit into from

Conversation

bengl
Copy link
Member

@bengl bengl commented Nov 11, 2015

Also some minor edits so the additions make sense.

I've used defineCommand several times now, figuring out how to use it by viewing the source. I figured it probably should be documented.

@thefourtheye thefourtheye added doc Issues and PRs related to the documentations. repl Issues and PRs related to the REPL subsystem. labels Nov 11, 2015
@@ -190,15 +194,59 @@ be emitted.
Example of listening for `reset`:

// Extend the initial repl context.
var r = repl.start({ options ... });
var replServer = repl.start({ options ... });
Copy link
Member

Choose a reason for hiding this comment

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

Nit: const instead of var?

Copy link
Member Author

Choose a reason for hiding this comment

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

Other pre-existing examples in the doc use var for such things, so I thought it was best to keep with the style already in this doc. Perhaps instead a future PR can convert them all in one fell swoop?

Copy link
Member

Choose a reason for hiding this comment

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

Sure. It's a nit. Ignore it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool beans.

@Trott
Copy link
Member

Trott commented Nov 11, 2015

LGTM with one typo correction that should happen and a bunch of nits that can be ignored if you wish because they are, you know, just nits.

@bengl
Copy link
Member Author

bengl commented Nov 11, 2015

Cool, yeah, I think the vars should stay for consistency until a full overhaul is done. Other than that, those nits are addressed now I think.

@jasnell
Copy link
Member

jasnell commented Nov 12, 2015

LGTM

Also some minor edits so the additions make sense.
@bengl
Copy link
Member Author

bengl commented Nov 13, 2015

OK, @Trott helpfully pointed out a conflict with master and it seems due to 8a245ea, but that was easy to resolve, so here's a rebased version. I also restored the link URLs to the bottom of the file.

@Trott
Copy link
Member

Trott commented Nov 13, 2015

LGTM

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 13, 2015
Also some minor edits so the additions make sense.

PR-URL: nodejs#3765
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott
Copy link
Member

Trott commented Nov 13, 2015

Landed in 061b2c8

@Trott Trott closed this Nov 13, 2015
@tflanagan
Copy link
Contributor

Not to be a doc-nazi, but could we put the class definition first, and then resort alpha?

@Trott
Copy link
Member

Trott commented Nov 14, 2015

@tflanagan Whoops! Did we just mess up a bit of the alphabetizing work that you and jasnell did? Sorry about that. Since this is now merged with master, we'll want a separate PR to re-alphabetize. Do you want to put together a PR on this file for it? I'm happy to do if you'd rather not.

@tflanagan
Copy link
Contributor

@Trott no worries! I figured this would happen once or twice :) I'll take care of it on Monday

Fishrock123 pushed a commit that referenced this pull request Nov 17, 2015
Also some minor edits so the additions make sense.

PR-URL: #3765
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2015
Also some minor edits so the additions make sense.

PR-URL: #3765
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins
Copy link
Contributor

landed in v4.x-staging in 5254fda

This was referenced Mar 22, 2022
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

Successfully merging this pull request may close these issues.

6 participants