Skip to content

testscript: expose (*TestScript).stdout via SetStdout(string)#217

Closed
myitcv wants to merge 1 commit intomasterfrom
expose_setstdout
Closed

testscript: expose (*TestScript).stdout via SetStdout(string)#217
myitcv wants to merge 1 commit intomasterfrom
expose_setstdout

Conversation

@myitcv
Copy link
Collaborator

@myitcv myitcv commented Apr 26, 2023

(see commit message)

Similarly, expose (*TestScript).stderr via SetStderr(string).

Closes #139
Copy link
Collaborator

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

SGTM with two nits:

  • Please add a test case for a command that writes to stdout and stderr, and then returns an error. I wonder if stderr is then kept at all, for example.
  • The wording for "updates the last stdout/stderr" feels slightly confusing; it feels like I could/should call it multiple times, and "last" isn't entirely correct when one is running the current command. How about "SetStdout can be used by commands in Params.Cmds to provide a resulting stdout string", since I don't believe we have another usecase yet?

@myitcv
Copy link
Collaborator Author

myitcv commented Apr 26, 2023

  • Please add a test case for a command that writes to stdout and stderr, and then returns an error. I wonder if stderr is then kept at all, for example.

I've replied to this point in #139 (comment) so we keep decisions in the issue (because it feels like I'm straying from the PR being "just" the implementation here).

  • The wording for "updates the last stdout/stderr" feels slightly confusing; it feels like I could/should call it multiple times, and "last" isn't entirely correct when one is running the current command. How about "SetStdout can be used by commands in Params.Cmds to provide a resulting stdout string", since I don't believe we have another usecase yet?

Will fix this as/when we agree on the API/approach.

@myitcv
Copy link
Collaborator Author

myitcv commented Apr 26, 2023

Closing in favour of #216

@myitcv myitcv closed this Apr 26, 2023
@myitcv myitcv deleted the expose_setstdout branch April 26, 2023 20:11
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.

2 participants