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 more documentation for $ #533

Merged
merged 8 commits into from Mar 6, 2023
Merged

Add more documentation for $ #533

merged 8 commits into from Mar 6, 2023

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented Mar 6, 2023

This PR adds more documentation for the $ scripting interface.

In particular, it presents its differences with using either Bash or zx.

Please note that some of the features being documented are related to open PRs still being discussed.

Direct link to documentation file in Markdown.

cc @aaronccasanova for code review.

@ehmicky ehmicky force-pushed the scripts-doc branch 4 times, most recently from 6c49fbf to f823ff5 Compare March 6, 2023 02:42
Copy link
Contributor

@aaronccasanova aaronccasanova left a comment

Choose a reason for hiding this comment

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

This looks great @ehmicky 🚀

docs/scripts.md Outdated Show resolved Hide resolved

```js
// zx
within(async () => {
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
within(async () => {
await within(async () => {

Does this need to be awaited?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This does not seem to be the case according to their documentation: https://github.com/google/zx/#within
Although according to their type, it would: https://github.com/google/zx/blob/4999435182fe1f34f6b2ef2471184d9878456b5e/src/core.ts#L436 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if that's a typo. I saw that and their second example including an await/assignment. Not a show stopper imo

readme.md Show resolved Hide resolved
docs/scripts.md Outdated
### Simplicity

Execa's scripting API mostly consists of only two methods: [``$`command` ``](../readme.md#command) and
[`$(options)`](../readme.md#options).
Copy link
Owner

Choose a reason for hiding this comment

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

Don't hard-wrap

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed 👍

docs/scripts.md Outdated Show resolved Hide resolved
docs/scripts.md Outdated Show resolved Hide resolved
docs/scripts.md Outdated
Comment on lines 205 to 206
import getStdin from 'get-stdin';
const content = await getStdin();
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
import getStdin from 'get-stdin';
const content = await getStdin();
import getStdin from 'get-stdin';
const content = await getStdin();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed 👍

@sindresorhus
Copy link
Owner

It would be useful to include an example of #528 in here too.

@ehmicky
Copy link
Collaborator Author

ehmicky commented Mar 6, 2023

Everything should be fixed! 🚀

An example of #528 is included in the "Subcommands" section

@ehmicky ehmicky requested review from aaronccasanova and sindresorhus and removed request for aaronccasanova March 6, 2023 17:05
@ehmicky
Copy link
Collaborator Author

ehmicky commented Mar 6, 2023

@aaronccasanova Do you think this might be good to merge?

@ehmicky ehmicky merged commit 9bcfa95 into main Mar 6, 2023
20 checks passed
@ehmicky ehmicky deleted the scripts-doc branch March 6, 2023 19:59
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