-
Notifications
You must be signed in to change notification settings - Fork 4k
api: Add a Supplier overload to Context #12935
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
Changes from 2 commits
275ad7d
55b44a9
f7faff8
6164702
afc76e4
01063b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,7 @@ | |
| import java.util.concurrent.TimeoutException; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
| import java.util.function.Supplier; | ||
| import java.util.logging.Handler; | ||
| import java.util.logging.Level; | ||
| import java.util.logging.LogRecord; | ||
|
|
@@ -553,6 +554,46 @@ public Object call() { | |
| current.detach(Context.ROOT); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSupply() throws Exception { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| Context base = Context.current().withValue(PET, "cat"); | ||
| Context current = Context.current().withValue(PET, "fish"); | ||
| current.attach(); | ||
|
|
||
| final Object ret = new Object(); | ||
| Supplier<Object> supplier = new Supplier<Object>() { | ||
| @Override | ||
| public Object get() { | ||
| runner.run(); | ||
| return ret; | ||
| } | ||
| }; | ||
|
|
||
| assertSame(ret, base.supply(supplier)); | ||
| assertSame(base, observed); | ||
| assertSame(current, Context.current()); | ||
|
|
||
| assertSame(ret, current.supply(supplier)); | ||
| assertSame(current, observed); | ||
| assertSame(current, Context.current()); | ||
|
|
||
| final TestError err = new TestError(); | ||
| try { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you feel strongly I can, but I was keeping the style of the tests around it.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| base.supply(new Supplier<Object>() { | ||
| @Override | ||
| public Object get() { | ||
| throw err; | ||
| } | ||
| }); | ||
| fail("Excepted exception"); | ||
| } catch (TestError ex) { | ||
| assertSame(err, ex); | ||
| } | ||
| assertSame(current, Context.current()); | ||
|
|
||
| current.detach(Context.ROOT); | ||
| } | ||
|
|
||
| @Test | ||
| public void currentContextExecutor() { | ||
| QueuedExecutor queuedExecutor = new QueuedExecutor(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually,
Callable<Void>shouldn't even need@CanIgnoreReturnValue. So I guess we did it just because there were existing callers.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I sent out #12955 to update the context version