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

refactor: return error on pipe from Wait method after completion of pipe #211

Merged
merged 9 commits into from
Aug 28, 2024

Conversation

mahadzaryab1
Copy link
Contributor

@mahadzaryab1 mahadzaryab1 commented Aug 18, 2024

Updated the Wait method to return the error present on the pipe after completion. This PR closes #123.

@mahadzaryab1 mahadzaryab1 changed the title feat: add WaitError method for returning error after completion of pipe feat: return error on pipe from Wait method after completion of pipe Aug 24, 2024
@mahadzaryab1 mahadzaryab1 changed the title feat: return error on pipe from Wait method after completion of pipe refactor: return error on pipe from Wait method after completion of pipe Aug 24, 2024
script_test.go Show resolved Hide resolved
script.go Outdated Show resolved Hide resolved
Copy link
Owner

@bitfield bitfield left a comment

Choose a reason for hiding this comment

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

I think this is about ready to go once you've rebased it on master!

script.go Outdated Show resolved Hide resolved
script_test.go Outdated
@@ -1850,6 +1850,22 @@ func TestReadReturnsErrorGivenReadErrorOnPipe(t *testing.T) {
}
}

func TestWaitForErrorOnPipe(t *testing.T) {
Copy link
Owner

Choose a reason for hiding this comment

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

If you try gotestdox here, as discussed on the other PR, you'll see why this test name doesn't quite fit with the others:

 ✔ Error returns error set by previous pipe stage (0.00s)
...

 ✔ Wait reads pipe source to completion (0.00s)
...
 ✔ Wait for error on pipe (0.00s)
 ✔ Wait for no error on pipe (0.00s)

See what I mean? It's not a sentence describing the effect that Wait has. (This is why I find gotestdox so useful as a thinking tool: it forces me to describe the behaviour of the thing I'm implementing.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the great suggestion @bitfield! changed the test names to conform to your suggestion. Let me know what you think.

Copy link
Owner

@bitfield bitfield left a comment

Choose a reason for hiding this comment

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

Great job! Thanks a lot.

@bitfield bitfield merged commit 0daf4b2 into bitfield:master Aug 28, 2024
8 checks passed
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.

ExecForEach().Error() doesn't execute
3 participants