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

TTY: Document WriteStream.cursorTo and others #22893

Closed
wants to merge 3 commits into from
Closed

Conversation

ur0
Copy link
Contributor

@ur0 ur0 commented Sep 17, 2018

This adds documentation for the following WriteStream instance methods:

  • cursorTo
  • moveCursor
  • clearLine
  • clearScreenDown
  • getWindowSize

Fixes #9853.

Checklist

Adds documentation for the following WriteStream instance methods:
- cursorTo
- moveCursor
- clearLine
- clearScreenDown
- getWindowSize

Fixes nodejs#9853.
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tty Issues and PRs related to the tty subsystem. labels Sep 17, 2018
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Hi @ur0. Thank you for an effort to improve the docs!

I do not know if these methods are intended to be documented but in case they do, I've mentioned some nits that can be fixed)

doc/api/tty.md Outdated
* `x` {number}
* `y` {number}

`writeStream.cursorTo` moves this `WriteStream`'s cursor to the specified
Copy link
Contributor

Choose a reason for hiding this comment

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

`writeStream.cursorTo` -> `writeStream.cursorTo()` as per our doc/STYLE_GUIDE.md. The same for the method mentions below.

doc/api/tty.md Outdated
`writeStream.cursorTo` moves this `WriteStream`'s cursor to the specified
position.

### writeStream.moveCursor(dx, dy)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doc sections need to be ABC-sorted by their headings.

doc/api/tty.md Outdated
<!-- YAML
added: v0.7.7
-->
* Returns: {array}
Copy link
Contributor

Choose a reason for hiding this comment

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

{array} -> {Array} (non-primitive types are uppercased).
Or maybe, as we know the array's elements type, we can make this {number[]}.

doc/api/tty.md Outdated
-->
* Returns: {array}

`writeStream.getWindowSize` returns the size of the [TTY]() corresponding
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the link should be [TTY](tty.html) or [TTY](#tty_tty) here and below.

doc/api/tty.md Outdated

`writeStream.getWindowSize` returns the size of the [TTY]() corresponding
to this `WriteStream`. The array is of the type `[numColumns, numRows]`
where `numColumns` and `numRows` represents the number of columns and rows
Copy link
Contributor

Choose a reason for hiding this comment

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

represents -> represent.

@vsemozhetbyt
Copy link
Contributor

cc @nodejs/documentation

@ur0
Copy link
Contributor Author

ur0 commented Sep 18, 2018

Thanks @vsemozhetbyt! I’ll fix the issues in a bit.

@addaleax
Copy link
Member

ping @ur0 … anything we can help with? :)

- Adds missing parens in function names
- Changes `array` to `number[]`
- Sorts headers
- Adds missing link
@ur0
Copy link
Contributor Author

ur0 commented Sep 25, 2018

Sorry for the delay, and thanks for the offer @addaleax!

Just fixed all of the nits. CC @vsemozhetbyt @nodejs/documentation for a review.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Looks like CI failed so that'll need a look but otherwise LGTM

doc/api/tty.md Outdated
to this `WriteStream`. The array is of the type `[numColumns, numRows]`
where `numColumns` and `numRows` represent the number of columns and rows
in the corresponding [TTY](tty.html).

## tty.isatty(fd)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this section should be the last one)

doc/api/tty.md Outdated
@@ -147,6 +164,17 @@ to pass in an object with different settings.
Use the `NODE_DISABLE_COLORS` environment variable to enforce this function to
always return 1.

## writeStream.getWindowSize()
Copy link
Contributor

Choose a reason for hiding this comment

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

## -> ###.

doc/api/tty.md Outdated
-->
* Returns: {number[]}

`writeStream.getWindowSize()` returns the size of the [TTY](tty.html) corresponding
Copy link
Contributor

Choose a reason for hiding this comment

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

This line exceeds 80 characters limit (linter fails here).

@vsemozhetbyt
Copy link
Contributor

@ur0 Thank you for the fixes and patience. Just a few nits remain to be addressed.

@ur0
Copy link
Contributor Author

ur0 commented Oct 5, 2018

@vsemozhetbyt all done, hopefully.

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 5, 2018
vsemozhetbyt pushed a commit that referenced this pull request Oct 5, 2018
Adds documentation for the following `WriteStream` instance methods:

- `WriteStream.clearLine()`
- `WriteStream.clearScreenDown()`
- `WriteStream.cursorTo()`
- `WriteStream.getWindowSize()`
- `WriteStream.moveCursor()`

PR-URL: #22893
Fixes: #9853
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
@vsemozhetbyt
Copy link
Contributor

Landed in 2ba19ff
(with nit fixed: ## tty.isatty(fd) level and position is restored).

Thank you!

targos pushed a commit that referenced this pull request Oct 6, 2018
Adds documentation for the following `WriteStream` instance methods:

- `WriteStream.clearLine()`
- `WriteStream.clearScreenDown()`
- `WriteStream.cursorTo()`
- `WriteStream.getWindowSize()`
- `WriteStream.moveCursor()`

PR-URL: #22893
Fixes: #9853
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
targos pushed a commit that referenced this pull request Oct 7, 2018
Adds documentation for the following `WriteStream` instance methods:

- `WriteStream.clearLine()`
- `WriteStream.clearScreenDown()`
- `WriteStream.cursorTo()`
- `WriteStream.getWindowSize()`
- `WriteStream.moveCursor()`

PR-URL: #22893
Fixes: #9853
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
Adds documentation for the following `WriteStream` instance methods:

- `WriteStream.clearLine()`
- `WriteStream.clearScreenDown()`
- `WriteStream.cursorTo()`
- `WriteStream.getWindowSize()`
- `WriteStream.moveCursor()`

PR-URL: #22893
Fixes: #9853
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
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. doc Issues and PRs related to the documentations. tty Issues and PRs related to the tty subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants