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

Improve documentation of stdin, stdout, stderr and stdio #626

Merged
merged 7 commits into from
Dec 21, 2023

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented Dec 20, 2023

This PR improves the documentation of the std* options.

@ehmicky ehmicky force-pushed the improve-docs-stdio branch 2 times, most recently from 7ad8ba3 to bcfe734 Compare December 20, 2023 01:26

#### stdin

Type: `string | number | stream.Readable | ReadableStream | undefined | URL | Iterable<string | Uint8Array> | AsyncIterable<string | Uint8Array>`\
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discourage using undefined: 'pipe' is clearer, and in most common cases is not needed since it is the default value.

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated
Same options as [`stdio`](https://nodejs.org/dist/latest-v6.x/docs/api/child_process.html#child_process_options_stdio).

It can also be a file path, a file URL, a web stream ([`ReadableStream`](https://developer.mozilla.org/en-US/docs/Web/API/ReadableStream)), an [`Iterable`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols#the_iterable_protocol) or an [`AsyncIterable`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols#the_async_iterator_and_async_iterable_protocols), unless either [`execaSync()`](#execasyncfile-arguments-options), the [`input` option](#input) or the [`inputFile` option](#inputfile) is used. If the file path is relative, it must start with `.`.
[How to setup](https://nodejs.org/dist/latest-v6.x/docs/api/child_process.html#child_process_options_stdio) the child process' standard input. This can be:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[How to setup](https://nodejs.org/dist/latest-v6.x/docs/api/child_process.html#child_process_options_stdio) the child process' standard input. This can be:
[How to setup](https://nodejs.org/docs/latest-v18.x/api/child_process.html#child_process_options_stdio) the child process' standard input. This can be:

Should this link point to the LTS docs? (Referencing pkg.engines.node)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing this out! Fixed.

index.d.ts Outdated
@@ -435,7 +474,8 @@ export type ExecaReturnValue<StdoutStderrType extends StdoutStderrAll = string>

This is `undefined` if either:
- the `all` option is `false` (default value)
- `execaSync()` was used
- the synchronous methods are used
- both `stdout` and `stderr` options are set to [`'inherit'`, `'ipc'`, `'ignore'`, `Stream` or `integer`](https://nodejs.org/dist/latest-v6.x/docs/api/child_process.html#child_process_options_stdio)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- both `stdout` and `stderr` options are set to [`'inherit'`, `'ipc'`, `'ignore'`, `Stream` or `integer`](https://nodejs.org/dist/latest-v6.x/docs/api/child_process.html#child_process_options_stdio)
- both `stdout` and `stderr` options are set to [`'inherit'`, `'ipc'`, `'ignore'`, `Stream` or `integer`](https://nodejs.org/dist/latest-v6.x/docs/api/child_process.html#child_process_options_stdio)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It appears that no using indentation shows like this with VSCode:

Screenshot from 2023-12-20 20-30-26

But using indentation shows like this:

Screenshot from 2023-12-20 20-30-44

However, we were being inconsistent with both the types and the readme.md, so I fixed it all to make it consistent.

index.d.ts Outdated
*/
shortMessage: string;

/**
Original error message. This is the same as the `message` property except it includes neither the child process stdout/stderr nor some additional information added by Execa.
Original error message. This is the same as the `message` property except it includes neither the child process `stdout`/`stderr` nor some additional information added by Execa.
Copy link
Contributor

Choose a reason for hiding this comment

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

Understanding this diff is a formatting update, the sentence reads a little confusing to me (particularly from except it includes neither [...]).

Light suggestion, but perhaps this is more clear:

This is the same as the message property excluding the child process stdout/stderr and additional information added by Execa.

Copy link
Collaborator Author

@ehmicky ehmicky Dec 20, 2023

Choose a reason for hiding this comment

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

Much better! Thanks.
Done.

@ehmicky
Copy link
Collaborator Author

ehmicky commented Dec 20, 2023

Ready for review again.
Note that based on upon review, there are now some documentation changes not strictly related to std* (e.g. indentation updates).

@ehmicky
Copy link
Collaborator Author

ehmicky commented Dec 21, 2023

Fixed the merge conflict.

@sindresorhus sindresorhus merged commit c405105 into main Dec 21, 2023
14 checks passed
@sindresorhus sindresorhus deleted the improve-docs-stdio branch December 21, 2023 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants