gnodet-bot commented on code in PR #13471:
URL: https://github.com/apache/maven/pull/13471#discussion_r4260429525
##########
impl/maven-di/src/main/java/org/apache/maven/di/Injector.java:
##########
@@ -157,7 +166,6 @@ static Injector create() {
* @param key the class representing the type to retrieve
* @return an instance of the requested type
* @throws NullPointerException if key is null
Review Comment:
⚠️ **`@throws` documentation regression (still open from
PRRT_kwDOAAMmk86r6NGa):** The `@throws IllegalStateException` that existed on
master was removed by this PR's first commit. The previous push added `@throws
DIException` as a replacement, but the latest commit removed it again (along
with the import). These methods now have zero failure-mode documentation —
callers reading only the Javadoc have no idea the method can throw.
If coupling the `Injector` interface to `DIException` (an impl-package type)
is the concern, consider `@throws RuntimeException if no binding is found or
bindings are ambiguous` as a package-neutral alternative. Either way, the
failure mode needs to be documented.
```suggestion
* @throws NullPointerException if key is null
* @throws DIException if no binding is found for the key, or if
bindings are ambiguous
```
##########
impl/maven-di/src/main/java/org/apache/maven/di/Injector.java:
##########
@@ -172,7 +180,6 @@ static Injector create() {
* @param key the key identifying the instance to retrieve
* @return an instance matching the requested key
* @throws NullPointerException if key is null
Review Comment:
⚠️ Same `@throws` documentation gap as the `getInstance(Class<T>)` overload
above.
```suggestion
* @throws NullPointerException if key is null
* @throws DIException if no binding is found for the key, or if
bindings are ambiguous
```
##########
impl/maven-di/src/main/java/org/apache/maven/di/impl/InjectorImpl.java:
##########
@@ -188,10 +189,36 @@ private Injector doBind(Key<?> key, Binding<?> binding) {
protected <U> Injector bind(Key<U> key, Binding<U> b) {
Set<Binding<?>> bindingSet = bindings.computeIfAbsent(key, $ -> new
HashSet<>());
- bindingSet.add(b);
+ if (bindingSet.stream().noneMatch(existing -> isSameBinding(existing,
b))) {
+ bindingSet.add(b);
+ }
return this;
}
+ private static boolean isSameBinding(Binding<?> first, Binding<?> second) {
+ if (!Objects.equals(first.getOriginalKey(), second.getOriginalKey())
+ || first.getPriority() != second.getPriority()
+ || !Objects.equals(first.getScope(), second.getScope())) {
+ return false;
+ }
+ Object firstIdentity = first.getRegistrationIdentity();
+ Object firstIdentity = first.getRegistrationIdentity();
Review Comment:
🚨 **Build-breaking duplicate variable declaration:** `Object firstIdentity`
is declared twice — on line 204 and again here on line 205 (both call
`first.getRegistrationIdentity()`). This is a compilation error: `variable
firstIdentity is already defined in method isSameBinding`. The explanatory
comment on lines 207–209 is correct and should be kept; only this duplicate
line needs to go.
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
