api: Add a Supplier overload to Context - #12935
Conversation
|
|
#12936 Appears to be needed. |
| assertSame(current, Context.current()); | ||
|
|
||
| final TestError err = new TestError(); | ||
| try { |
There was a problem hiding this comment.
TestError caught = assertThrows(TestError.class, ...
There was a problem hiding this comment.
If you feel strongly I can, but I was keeping the style of the tests around it.
There was a problem hiding this comment.
assertThrows() was not available back then, worth using in new code
kannanjgithub
added
the
kokoro:force-run
grpc-kokoro
removed
the
kokoro:force-run
|
@kannanjgithub @ejona86 Looks like context is Java7? Any chance Line 19 in 3b3002c |
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
supply() is a bit of a weird name. But get() would be even weirder.
Mark it experimental like normal? (Just with a comment, since we can't use the annotation here.)
Yeah, I think we could drop Java 7 and compile with Java 8. It's been 4.5 years since we dropped Java 7. We should probably keep the separate compilation step as when we drop Java 8 we'll probably want Context to again stay on Java 8 for a while.
| * @param supplier {@link Supplier} to use to produce the value. | ||
| * @return result of supplier. | ||
| */ | ||
| @CanIgnoreReturnValue |
There was a problem hiding this comment.
This seems a bit strange. Did we only ignore call() because of existing callers? Hey, look! @carl-mastrangelo reviewed #4430 😛
I guess people could be doing Callable<Void> if they wanted a Runnable that could throw exceptions. I don't think there's any point to ignoring a Supplier's return value though.
There was a problem hiding this comment.
Actually, Callable<Void> shouldn't even need @CanIgnoreReturnValue. So I guess we did it just because there were existing callers.
There was a problem hiding this comment.
Definitely don't like the name supply, but also can't really use an overload of call. I'm open to alternatives. invoke() ? callInContext() ? apply() ?
There was a problem hiding this comment.
Also I sent out #12955 to update the context version
AgraVator
added
the
kokoro:force-run
grpc-kokoro
removed
the
kokoro:force-run
|
@kannanjgithub Friendly ping on this. |
| } | ||
|
|
||
| @Test | ||
| public void testSupply() throws Exception { |
There was a problem hiding this comment.
This single test is testing multiple different behaviors. We can rewrite it into 3 different behavior driven tests each following the Given-When-Then structure for testing each behavior:
supply_executesSupplierUnderTargetContextAndRestoresPrevious
supply_whenContextIsAlreadyCurrent_executesAndMaintainsContext
supply_whenSupplierThrows_propagatesExceptionAndRestoresPreviousContext
There was a problem hiding this comment.
I think it would be better for the maintainers to take on that task. I wrote the test to match the existing style, and since I don't know your preferences, I think it would be better for you to adjust it the way you want.
Claude should be able to easily adjust and split these tests. How about we continue with the PR as is and the tests can be adjusted after?
6 participants
Fixes #12927