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

README.md: more examples of (mis)using run #366

Merged
merged 3 commits into from Nov 11, 2022

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Oct 9, 2020

This is a followup to #343 and closes #377.

After fixing some more code that uses run left and right, I am coming back
to further improve its documentation.

In particular, I try to emphasize the only use case for run (checking the output
of a command that exits with non-zero status).

I also provide a non-working example of using run with pipes.

@martin-schulze-vireso
Copy link
Member

Nice. I think that ties in well with #358.

@kolyshkin kolyshkin mentioned this pull request Oct 13, 2020
2 tasks
@kolyshkin
Copy link
Contributor Author

@sublimino PTAL

@sublimino
Copy link
Member

Really nice work! These clear, simply stated documentation changes make the basics much easier for newcomers to bats, thanks @kolyshkin! You mentioned above:

In particular, I try to emphasize the only use case for run (checking the output
of a command that exits with non-zero status).

I use run with https://github.com/bats-core/bats-assert#assert_output a lot, so I've added a "subshell" method to the end of the list in e0e13c3 too 🙏

@martin-schulze-vireso
Copy link
Member

Wow, I think this closes #1.

@martin-schulze-vireso
Copy link
Member

With the discussion going on in #367, I would want to wait for a decision there, to avoid landing this and making it obsolete in the next step.

@sublimino
Copy link
Member

@martin-schulze-vireso good point, I missed that one somehow! Will have a look over the weekend.

It could be worth merging this so there's a baseline to extend upon with #367, but don't mind either way.

@martin-schulze-vireso
Copy link
Member

martin-schulze-vireso commented Nov 6, 2020

Just noticed that the above PR already tried to document some of this. We should

@martin-schulze-vireso
Copy link
Member

It could be worth merging this so there's a baseline to extend upon with #367, but don't mind either way.

I hope that when #367 is done, there is only one use case without run: Check success + get output unconditionally.

@martin-schulze-vireso
Copy link
Member

martin-schulze-vireso commented Aug 7, 2021

@kolyshkin Now that #367 has landed (although the exact syntax is not settled #479), how much of this PR should we retain?

We should also consider that #467 will offer switches to alter the output behavior of run, which might complicate this further.

@kolyshkin
Copy link
Contributor Author

I agree that (once we settle on "run with exit code check" syntax) this needs to be revised, as now there are more ways to achieve the same result.

Yet I think the documentation removal in #367 (e.g. ae26797#diff-12cfeed9f9b1cc83679dd5e693fdf109f2080614ae71c90466cace54a187082dL54) is a mistake.

Instead, it should have been modified to show how to check exit codes.

For example, writing

run command args ...
echo "$output"
[ "$status" -eq 0 ]

(the first example from the removed doc) is OK but not a good style; either command args ... or run 0 command args ... is better.

@martin-schulze-vireso
Copy link
Member

@kolyshkin You're right. I removed that part and intended to add new documentation for it but never did. We'll let this reminder sit here until we settled on the syntax.

@martin-schulze-vireso
Copy link
Member

martin-schulze-vireso commented Feb 24, 2022

@kolyshkin Would you mind having another stab at this? The run -X/run ! syntax is released now, so we can move forward.

@martin-schulze-vireso
Copy link
Member

@kolyshkin I did not hear back from you. Do you plan to work on this again?

@martin-schulze-vireso
Copy link
Member

I reworked this according to the new run exit code check interface and added automatic output/stderr printing on exit code errors to run. This changed in the documentation in comparison to the old code:

  1. "check the command succeeded": updated command to use run's exit code check
  2. "check the command failed": replaced by reference to run !
  3. hide the command output: omitted, with new run exit code checking, the run command is actually shorter
  4. make sure the command succeeded, and check its output: omitted, run -0 is shorter
  5. "pipe using run": made an extra section instead of bullet point. However, the recommendation is about to change with Adding bats_pipe Helper Function (originally suggested adding --output-processor Flag to run Helper). #663, so we might want to reword that later on

@kolyshkin
Copy link
Contributor Author

@martin-schulze-vireso thank you, I was just checking bats-core docs and was glad to see these examples. Sorry for not following up on this myself.

@kolyshkin
Copy link
Contributor Author

@martin-schulze-vireso I would also appreciate adding "Co-authored-by" footer next time 😉

@martin-schulze-vireso
Copy link
Member

@martin-schulze-vireso I would also appreciate adding "Co-authored-by" footer next time 😉

You mean in the commit message? I am sorry that this got swept away in the many revisions this has undergone. Usually I can rely on rebase to preserve this.

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.

Improve the run function
3 participants