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 .escapedCommand property #466

Merged
merged 7 commits into from Jun 2, 2021
Merged

Add .escapedCommand property #466

merged 7 commits into from Jun 2, 2021

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented Jun 1, 2021

Closes #455.

This adds a debugString property to the return value and error instances, as described in the above issue.

As I implemented this, I realized that this property is very similar to the existing command property. What are your thoughts on either of the following options?

  1. Rename debugString to escapedCommand or quotedCommand to reflect this.
  2. Add escaping to the command property instead. This would be a breaking change.

@borekb
Copy link

borekb commented Jun 1, 2021

Great to see this happening! I'm a fan of being able to pass the debugString (possibly under a different name) to execa.command and see it work. This was the example in #455 (comment):

const execa = require('execa');

(async () => {
  const { command } = await execa('echo', ['unicorns']);
  await execa.command(command); // works
})();

debugString is a good name but I like command as well, and it would be an intuitive counterpart to execa.command.

@ehmicky
Copy link
Collaborator Author

ehmicky commented Jun 1, 2021

Great to see this happening! I'm a fan of being able to pass the debugString (possibly under a different name) to execa.command and see it work. This was the example in #455 (comment):

The discussion in #455 resolved towards a different purpose though: returning a string for debugging purpose (per @sindresorhus comment here), not to re-use programmatically. Re-using the same command + arguments programmatically can already be achieved by using variables to store them, or by refactoring similar code into single functions (see my previous comment).

debugString is a good name but I like command as well, and it would be an intuitive counterpart to execa.command.

This might actually be an argument against using the name command since debugString cannot be passed as is to execa.command() since debugString uses some naive shell-like escaping for debugging purpose based on double quotes, while execa.command() uses no escaping except for spaces which are backslash-escaped.

@borekb
Copy link

borekb commented Jun 1, 2021

My comment was specifically about your no. 2 suggestion above, i.e., changing execa.command so that it accepts the "debug string". If that was the case, I think that the "debug string" could be called "command".

But I understand it's slightly controversial, and all your suggestions like debugString, quotedCommand or escapedCommand sound good to me.

@ehmicky
Copy link
Collaborator Author

ehmicky commented Jun 1, 2021

My comment was specifically about your no. 2 suggestion above, i.e., changing execa.command so that it accepts the "debug string". If that was the case, I think that the "debug string" could be called "command".

My bad, I am realizing my comment was not clear. By command, I meant the property that is returned by execa(), not the execa.command() method itself, nor changing execa.command() so it accepts debugString.

So by Add escaping to the command property instead., I meant merging the debugString and command properties into a single one named command. However, that idea might not be a good one since it might hint that the property can be passed to execa.command() although this is not the case.

@borekb
Copy link

borekb commented Jun 1, 2021

Got it, thanks for the explanation, it was at least partially a misunderstanding on my end 😄.

@sindresorhus
Copy link
Owner

I would go with renaming it to escapedCommand. I would also update the docs for both command and escapedCommand to make it clear that it should not be passed to execa.command, and that .command is for logging and .escapedCommand is for debugging in an actual shell.

@ehmicky
Copy link
Collaborator Author

ehmicky commented Jun 2, 2021

Fixed 👍
Please let me know what you think.

@ehmicky ehmicky changed the title Add debugString Add escapedCommand Jun 2, 2021
index.d.ts Outdated
Same as `command` but escaped.

This is meant to be copy/pasted in a shell, for debugging purpose.
Since the escaping is fairly basic, this should be passed to neither `execa()` nor `execa.command()`
Copy link
Owner

Choose a reason for hiding this comment

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

To prevent any vulnerability report coming from this, I think we should say that it should not be used with any child process type method. Same with .command.

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 👍

index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title Add escapedCommand Add .escapedCommand property Jun 2, 2021
ehmicky and others added 3 commits June 2, 2021 19:05
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
@ehmicky ehmicky requested a review from sindresorhus June 2, 2021 17:08
@sindresorhus sindresorhus merged commit 712bafc into main Jun 2, 2021
@sindresorhus sindresorhus deleted the feat/debug-string branch June 2, 2021 17:29
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.

Print the executed command?
3 participants