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 an overload of Timer#time that takes Runnable #989

Merged
merged 1 commit into from Jan 5, 2017

Conversation

Projects
None yet
5 participants
@stevenschlansker
Contributor

stevenschlansker commented Jul 21, 2016

This allows you to invoke method handles or lambdas that
don't return a value easily.

Add an overload of Timer#time that takes Runnable
This allows you to invoke method handles or lambdas that
don't return a value easily.

@arteam arteam merged commit 15dde82 into dropwizard:3.2-development Jan 5, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Jan 5, 2017

Member

Thanks for your contribution, Steven.

Member

arteam commented Jan 5, 2017

Thanks for your contribution, Steven.

@arteam arteam added the improvement label Jan 5, 2017

@arteam arteam added this to the 3.2.0 milestone Jan 5, 2017

@stevenschlansker stevenschlansker deleted the stevenschlansker:timer-runnable branch Jan 5, 2017

@mwhipple

This comment has been minimized.

Show comment
Hide comment
@mwhipple

mwhipple Mar 6, 2017

I may be missing something (and I'm definitely late) but this seems like a potentially bad design change. Now #time accepts 2 possible SAM argument types which will complicate implicit coercion of lambdas or similar first class functions.

mwhipple commented Mar 6, 2017

I may be missing something (and I'm definitely late) but this seems like a potentially bad design change. Now #time accepts 2 possible SAM argument types which will complicate implicit coercion of lambdas or similar first class functions.

@samperman

This comment has been minimized.

Show comment
Hide comment
@samperman

samperman Mar 6, 2017

Contributor

FYI, this breaks groovy users of this api. Code like this appears to always call the Runnable version and doesn't return anything:

def result = timer.time {
  doSomething()
}

A workaround is to modify the call to look like this:

def result = timer.time ({
  doSomething()
} as Callable)
Contributor

samperman commented Mar 6, 2017

FYI, this breaks groovy users of this api. Code like this appears to always call the Runnable version and doesn't return anything:

def result = timer.time {
  doSomething()
}

A workaround is to modify the call to look like this:

def result = timer.time ({
  doSomething()
} as Callable)
@ryantenney

This comment has been minimized.

Show comment
Hide comment
@ryantenney

ryantenney Mar 6, 2017

Member

@mwhipple I don't think that had been considered. We could revert this change as it's likely no one is using it yet. Either way we'll be sure to avoid this problem in v4.

Member

ryantenney commented Mar 6, 2017

@mwhipple I don't think that had been considered. We could revert this change as it's likely no one is using it yet. Either way we'll be sure to avoid this problem in v4.

@mwhipple

This comment has been minimized.

Show comment
Hide comment
@mwhipple

mwhipple Mar 6, 2017

@ryantenney cool, thanks for the quick response!

mwhipple commented Mar 6, 2017

@ryantenney cool, thanks for the quick response!

@ryantenney

This comment has been minimized.

Show comment
Hide comment
@ryantenney

ryantenney Mar 6, 2017

Member
Member

ryantenney commented Mar 6, 2017

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Mar 6, 2017

Member

I had an impression that the compiler is smart enough to figure out which SAM should applied depending on the argument. For instance, this snippet compiles for me:

Timer timer = new MetricRegistry().timer("test");
timer.time(() -> System.out.println());
timer.time(() -> System.currentTimeMillis());
Member

arteam commented Mar 6, 2017

I had an impression that the compiler is smart enough to figure out which SAM should applied depending on the argument. For instance, this snippet compiles for me:

Timer timer = new MetricRegistry().timer("test");
timer.time(() -> System.out.println());
timer.time(() -> System.currentTimeMillis());
@mwhipple

This comment has been minimized.

Show comment
Hide comment
@mwhipple

mwhipple Mar 6, 2017

Ok, thanks for the additional info...it looks like this may just be a Groovy issue.
http://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.12.2.1

If the target type's function type has a void return, then the lambda body is either a statement expression (§14.8) or a void-compatible block (§15.27.2).

If the target type's function type has a (non-void) return type, then the lambda body is either an expression or a value-compatible block (§15.27.2).

Sorry for the half-baked concern

mwhipple commented Mar 6, 2017

Ok, thanks for the additional info...it looks like this may just be a Groovy issue.
http://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.12.2.1

If the target type's function type has a void return, then the lambda body is either a statement expression (§14.8) or a void-compatible block (§15.27.2).

If the target type's function type has a (non-void) return type, then the lambda body is either an expression or a value-compatible block (§15.27.2).

Sorry for the half-baked concern

@stevenschlansker

This comment has been minimized.

Show comment
Hide comment
@stevenschlansker

stevenschlansker Mar 7, 2017

Contributor

If it is really that bad, you could just give the method a different name so it is not an overload.

Contributor

stevenschlansker commented Mar 7, 2017

If it is really that bad, you could just give the method a different name so it is not an overload.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment