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

Add execution result to Successful and ExceededTimeout events #2

Merged
merged 3 commits into from
Feb 23, 2014
Merged

Add execution result to Successful and ExceededTimeout events #2

merged 3 commits into from
Feb 23, 2014

Conversation

orrsella
Copy link
Contributor

Enables additional reporting/actions to be performed based on the calculated value.

orrsella added a commit that referenced this pull request Feb 23, 2014
Add execution result to Successful and ExceededTimeout events
@orrsella orrsella merged commit c726391 into wix-incubator:master Feb 23, 2014
@orrsella orrsella deleted the result-in-events branch February 23, 2014 14:31
case class ExceededTimeout(actual: Duration, executionName: String, params: Map[String, String]) extends Event
case class Successful(elapsed: Duration, executionName: String, params: Map[String, String]) extends Event
case class ExceededTimeout(actual: Duration, executionName: String, result: Any) extends Event
case class Successful(elapsed: Duration, executionName: String, result: Any) extends Event

Choose a reason for hiding this comment

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

This is what we discussed, right? I wouldn't go with Any here -- this will require implements to either use .asInstanceOf[T] or else pattern match on case Successful( _, _, result: T ) => .... Both are boilerplate-y and can fail at runtime. What about the "state" type solution we discussed?

@orrsella
Copy link
Contributor Author

Yes, this is what we discussed. If you care about the type of the result in the event, then you can match with the type as you said. Otherwise, you can match on case Successful(_, _, _). I agree that this is boilerplate-y.

I'm not sure if the current model fits the state we discussed. The Event class isn't extended in your own code, and neither is the Reporter.

Let's discuss in person.

@holograph
Copy link

Sure.

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

2 participants