Java: Nullable to Optional #1401
Conversation
|
@johnoliver any thoughts on this one? |
github-actions
bot
added
.NET
…nel into nullable-to-optional
github-actions
bot
removed
kernel.core
kernel
lemillermicrosoft
added
feature branch
|
So... I'm not a big fan of this one. The litmus test for Optional is whether the API is meant to be used in a stream/fluent way. Even then, you can do without Optional. If you look at the changes you have here, you see a lot of the |
| @@ -135,7 +135,7 @@ public Mono<CompletionSKContext> aggregatePartitionedResultsAsync( | |||
| return results.map( | |||
| list -> | |||
| list.stream() | |||
| .map(SKContext::getResult) | |||
| .map((_context) -> _context.getResult().get()) | |||
There was a problem hiding this comment.
This will blow up with NoSuchElementException if getResult on one of the elements returns Optional.empty(). I think a better way to do this would be
list.stream().map(SKContext::getResult).stream().collect(Collectors.joining("\n"))).map(context::update);
| public String get(String key) { | ||
| return variables.get(key); | ||
| @Nonnull | ||
| public Optional<String> get(String key) { |
There was a problem hiding this comment.
I think APIs that are of the "get the value of this key" variety should return null if the key is not found. Returning null from this Map-like method call is expected behavior.
| // If this is bound to input get the input value | ||
| if (inputArgs.contains(parameter)) { | ||
| String input = context.getVariables().get(ContextVariables.MAIN_KEY); | ||
| if (input != null && !input.isEmpty()) { | ||
| Optional<String> input = context.getVariables().get(ContextVariables.MAIN_KEY); |
There was a problem hiding this comment.
You lost the !input.isEmpty() check.
| arg = input; | ||
| } | ||
| } | ||
|
|
||
| if (arg == null || arg.isEmpty()) { | ||
| if (arg.isPresent() == false) { |
| } | ||
| } | ||
| } | ||
|
|
||
| if ((arg == null || arg.isEmpty()) && variableName.matches("arg\\d")) { | ||
| if (arg.isPresent() == false && variableName.matches("arg\\d")) { |
There was a problem hiding this comment.
Optional#isEmpty() is the same as Optional#isPresent() == false.
Your missing the 'arg.isEmpty()' (as in String#isEmpty()) check here.
| @@ -270,10 +270,10 @@ private static String getArgumentValue( | |||
| + " or @SKFunctionInputAttribute."); | |||
| } | |||
|
|
|||
| if (NO_DEFAULT_VALUE.equals(arg)) { | |||
| if (NO_DEFAULT_VALUE.equals(arg.get())) { | |||
There was a problem hiding this comment.
Shouldn't an empty optional at this point mean 'no default value'?
| it -> { | ||
| return it.getResult(); | ||
| }); | ||
| return result.map(it -> it.getResult().get()); |
There was a problem hiding this comment.
~Probably want result.map(it::getResult).orElseGet(Mono::empty)~ Yeah... my suggestion isn't right. Still, though, get()` will blow up.
Maybe return result.map(SKContext::getResult).filter(Optional::isPresent).map(Optional::get);
|
Thanks @dsgrieve for the review. I am ready to give up on this proposal as the benefits are definitely not greater than the disadvantages (as experienced by the introduction of bugs). |
No description provided.