Switch to JSpecify annotations for the model module #5129
Conversation
sdeleuze
left a comment
sdeleuze
left a comment
There was a problem hiding this comment.
Mostly good, just a few things to refine from my POV.
See https://jspecify.dev/docs/user-guide/#declaring-generics for my comments about generic types.
| public ChatResponse(List<Generation> generations, ChatResponseMetadata chatResponseMetadata) { | ||
| this.chatResponseMetadata = chatResponseMetadata; | ||
| Assert.notNull(generations, "'generations' must not be null"); | ||
| this.chatResponseMetadata = Objects.requireNonNullElse(chatResponseMetadata, new ChatResponseMetadata()); |
There was a problem hiding this comment.
Is it a defensive check because we want to be lenient if chatResponseMetadata is forced to null?
| return List.of(); | ||
| } | ||
|
|
||
| if (!StringUtils.hasText(context.getResponse().getResult().getOutput().getText())) { |
There was a problem hiding this comment.
Could you please share more details about why this check can be safely removed?
There was a problem hiding this comment.
Because the stream later below ends up in the same result
|
|
||
| targetMap.putAll(sourceMap.entrySet() | ||
| .stream() | ||
| .filter(e -> e.getValue() != null) |
There was a problem hiding this comment.
I think we kept a similar defensive filtering in a previous PR, is this one fine to remove?
| } | ||
|
|
||
| var nullableAnnotation = parameter.getAnnotation(Nullable.class); | ||
| var nullableAnnotation = parameter.getAnnotation(org.springframework.lang.Nullable.class); |
There was a problem hiding this comment.
Here you should probably use org.springframework.core.Nullness that I have introduced for this kind of use case.
|
|
||
| @Override | ||
| public String apply(String template, Map<String, Object> variables) { | ||
| public String apply(String template, Map<String, @Nullable Object> variables) { |
There was a problem hiding this comment.
Should probably be Map<String, ? extends @Nullable Object> to allow both Map<String, @Nullable Object> and Map<String, Object> (flexible nullability).
|
|
||
| @Override | ||
| public String apply(String template, Map<String, Object> variables) { | ||
| public String apply(String template, Map<String, @Nullable Object> variables) { |
There was a problem hiding this comment.
Should probably be Map<String, ? extends @Nullable Object> to allow both Map<String, @Nullable Object> and Map<String, Object> (flexible nullability).
| * @since 1.0.0 | ||
| */ | ||
| public interface TemplateRenderer extends BiFunction<String, Map<String, Object>, String> { | ||
| public interface TemplateRenderer extends BiFunction<String, Map<String, @Nullable Object>, String> { |
There was a problem hiding this comment.
Should probably be Map<String, ? extends @Nullable Object> to allow both Map<String, @Nullable Object> and Map<String, Object> (flexible nullability).
Same below.
| @SuppressWarnings("unchecked") | ||
| public static BiFunction<?, ToolContext, ?> wrapKotlinBiFunction(Object bean) { | ||
| return (t, u) -> ((Function2<Object, ToolContext, Object>) bean).invoke(t, u); | ||
| public static BiFunction<?, @Nullable ToolContext, ?> wrapKotlinBiFunction(Object bean) { |
There was a problem hiding this comment.
Should probably be ? extends @Nullable ToolContext (flexible nullability).
Same below and above.
| private final Type toolInputType; | ||
|
|
||
| private final BiFunction<I, ToolContext, O> toolFunction; | ||
| private final BiFunction<I, @Nullable ToolContext, O> toolFunction; |
There was a problem hiding this comment.
Should probably be ? extends @Nullable ToolContext (flexible nullability).
Same below.
| private @Nullable ToolMetadata toolMetadata; | ||
|
|
||
| private BiFunction<I, ToolContext, O> toolFunction; | ||
| private final BiFunction<I, @Nullable ToolContext, O> toolFunction; |
There was a problem hiding this comment.
Should probably be ? extends @Nullable ToolContext (flexible nullability).
Same below.
ericbottard
left a comment
ericbottard
left a comment
There was a problem hiding this comment.
Will merge addressing a few comments, and will revisit the "flexible nullability" topic when we can discuss live.
| return List.of(); | ||
| } | ||
|
|
||
| if (!StringUtils.hasText(context.getResponse().getResult().getOutput().getText())) { |
There was a problem hiding this comment.
Because the stream later below ends up in the same result
| } | ||
|
|
||
| var nullableAnnotation = parameter.getAnnotation(Nullable.class); | ||
| var nullableAnnotation = parameter.getAnnotation(org.springframework.lang.Nullable.class); |
Signed-off-by: Eric Bottard <eric.bottard@broadcom.com>
ericbottard
force-pushed
the
jspecify-2
branch
from
372cb38 to
594b661
Compare
2 participants
Thank you for taking time to contribute this pull request!
You might have already read the contributor guide, but as a reminder, please make sure to:
git commit -s) per the DCOmainbranch and squash your commitsFor more details, please check the contributor guide.
Thank you upfront!