Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[#29772][Go SDK] Add EventTime Timer tests. #29829
[#29772][Go SDK] Add EventTime Timer tests. #29829
Changes from 4 commits
2ac0ad2
fefb6a1
41dbc97
da8b533
7b304de
b8b5350
1fe6f00
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May we consider the following? (Blame the comment lines alignment on the GitHub comment editor).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validator is actually constructing the pipeline. It's not validating a pipeline. The validation has to be built in. It's not clear what benefit the additional indirection through a type (which is still exposed as a
func(s beam.Scope)
anyway serves here, and it's not going to be obvious a "Validator" is actually the same as expected byptest.BuildAndRun
, even though the compiler is happy with it.I agree that the current TimersEventTime builder function isn't ideal. I'm going to unexport it, and simply move the full pipeline builds "internally", so it's not exposed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how you re-worded the comment. It's more clear now. Feel free to resolve this conversation if you'd like.