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

[BEAM-2808] Improve error message when DoFn @ProcessElement has the wrong window type #3857

Closed
wants to merge 2 commits into from

Conversation

youngoli
Copy link
Contributor

Modifying the error message from this:

expects window type , which is not a supertype of actual window type

to this:

unable to provide window -- expected window type from parameter () is not a supertype of actual window type assigned by windowing ()

@lukecwik, can you review?

Copy link
Member

@lukecwik lukecwik left a comment

Choose a reason for hiding this comment

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

LGTM

@lukecwik
Copy link
Member

Note that most people automatically have the standard template and keep it part of the review.
See #3856 as an example.

We also took on the convention of asking for a specific reviewer with
"R: @username" and to bring attention to this PR for a specific user with "CC: @username"
This convention has not been codified within the contribution guide and would be useful to add there if you don't mind.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.5% when pulling dea7271 on youngoli:bugfix into 979923a on apache:master.

@lukecwik
Copy link
Member

Test failed, seems like you missed updating a test expectation.

@asfgit asfgit closed this in bd0facc Sep 15, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 69.514% when pulling 5980ba2 on youngoli:bugfix into 979923a on apache:master.

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.

None yet

3 participants