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

feature request: trim option #690

Closed
Zamiell opened this issue Oct 25, 2023 · 11 comments · Fixed by #737
Closed

feature request: trim option #690

Zamiell opened this issue Oct 25, 2023 · 11 comments · Fixed by #737
Labels

Comments

@Zamiell
Copy link

Zamiell commented Oct 25, 2023

Hello, and thanks for the very useful library!

The Problem

Consider the following simple script:

const branch = await $`git branch --show-current`;
if (branch.toString() !== "main") {
  throw new Error("You must run this script on the main branch only.")
}

However, this has a subtle pitfall. The output of the branch variable is actually going to be main\n. So our actual script has to be something like this:

const branch = await $`git branch --show-current`;
if (branch.toString().trim() !== "main") {
  throw new Error("You must run this script on the main branch only.")
}

Yuck! This adds boilerplate to scripts that we want to keep small and tidy.


Proposed Solution

Currently, zx is configurable with some options by modifying the $ object, like e.g.:

$.verbose = false;

Thus, I propose a new setting called trim (or maybe trimOutput), that would be set to false by default for backwards compatibility, but could be changed to true like this:

$.trim = true;

Once trim is set to true, then the code from the original snippet would work properly without any additional boilerplate.

If you think this is a good idea, I can try to do a PR.

@antonmedv
Copy link
Collaborator

I also was thinking about something like this at the beginning. But I decided what explicit is better.

Maybe an additional method should be introduced. Like

const branch = await $`git branch --show-current`;
if (branch.toTrimmedStdoutString() !== "main") {
  throw new Error("You must run this script on the main branch only.")
}

Or something shorter, a nice name is difficult to find.

@Zamiell
Copy link
Author

Zamiell commented Oct 26, 2023

toTrimmedString seems fine.

@antonmedv
Copy link
Collaborator

What about just trim output inside toString()?

@Zamiell
Copy link
Author

Zamiell commented Oct 26, 2023

Yes, that would be optimal, because the code in my first snippet in the OP would just work without having to do anything additional. And then, to get the non-trimmed output, one could access branch.stdout.

Additionally, right now, ProcessOutput.stdout and ProcessOutput.stderr do not have any JSDoc documentation. This "\n" pitfall is something that should be added as a mouseover in VSCode IMO.

@antonmedv
Copy link
Collaborator

I like this idea. What do you think @antongolub ?

@antongolub
Copy link
Contributor

I'd definitely support this enhancement.

image

@antongolub
Copy link
Contributor

antongolub commented Oct 26, 2023

We can implement a custom valueOf for the ProcessOutput.

@antongolub
Copy link
Contributor

antongolub commented Oct 26, 2023

As a workaround for now

import {ProcessOutput, $} from 'zx'

ProcessOutput.prototype.valueOf = function () {
  return this.toString()
}

ProcessOutput.prototype.toString = function () {
  const str = this._combined.toString()
  return $.trim
    ? str.trim()
    : str
}
// ...
{
  const output = await $`echo foobar`

  assert(output.toString().trim() === 'foobar')
  assert(output == 'foobar')
}

@antonmedv
Copy link
Collaborator

I like the idea of valueOf().

@antonmedv
Copy link
Collaborator

Added to the plan for zx v8(#589).

But should toString() also return trimmed output? I think we can make this "breaking change" in v8.

@antonmedv
Copy link
Collaborator

I'm against adding another configuration option $.trim

antongolub added a commit to antongolub/zx that referenced this issue Mar 18, 2024
antonmedv pushed a commit that referenced this issue Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@antonmedv @antongolub @Zamiell and others