Skip to content

Conversation

@holdenk
Copy link
Contributor

@holdenk holdenk commented Aug 6, 2018

Improve error reporting in calls.go by outputing the expected and provided number of arguments when a mismatch occurs.

Follow this checklist to help us incorporate your contribution quickly and easily:

  • [ X ] Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
--- --- --- ---

@holdenk
Copy link
Contributor Author

holdenk commented Aug 6, 2018

cc @herohde thanks for your help with finding the specialize thing, do you have cycles to review this change?

if c.Type().NumIn() != {{$in}} || c.Type().NumOut() != {{$out}} {
panic("incompatible func type")
panic(fmt.Sprintf("incompatible func type, expected function of {{$in}} " +
"inputs & {{$out}} outputs and instead received a function of %d inputs and %d" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Good change.

Small suggestion: maybe include the full function type as well? Btw, the usual way of writing these error messages in the opinionated language of Go is a terse got-then-want style: "blah: %v, want %v".

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to this being a good change, and I like henning's suggestion.
I agree that we should also include the type signature of the failing function since it'll help narrow down where something went wrong if the stack trace gets swallowed.

%v and passing in c.Type() will accomplish that.

Maybe:

panic(fmt.Sprintf("incompatible func type: %v has %d %d inputs and %d outputs, " + " want function of {{$in}} inputs & {{$out}} outputs", c.Type(), c.Type().NumIn(), c.Type().NumOut()))

@huygaa11
Copy link
Contributor

@herohde friendly ping!

@herohde
Copy link
Contributor

herohde commented Sep 13, 2018

@huygaa11 I'll wait for Holden to respond to my suggestion.

cc: @lostluck

@aaltay
Copy link
Member

aaltay commented Sep 27, 2018

What is the status of this PR?

@akedin
Copy link
Contributor

akedin commented Oct 9, 2018

This PR was last updated more than 2 months ago. We should either merge or close it soon.

@stale
Copy link

stale bot commented Dec 8, 2018

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.

@stale stale bot added the stale label Dec 8, 2018
@stale
Copy link

stale bot commented Dec 15, 2018

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

@stale stale bot closed this Dec 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants