Adding missing @Override annotations (part 5). Adding RRA/RWA/RUI annotations. Some localization and refactoring - #15
Conversation
|
@claude review pr with inline comments |
|
Claude finished @unv-unv's task in 3m 4s —— View job PR Review — complete ✅
I posted a review with inline suggestions (3 one-click SummaryThis is an overwhelmingly mechanical and clean pass —
Inline suggestions (all minor)
Worth double-checking (not blocking)Several sites swap No behavioral bugs found. 👍 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review — part 5 (@OverRide / RRA-RWA-RUI / localization / refactoring)
This is an overwhelmingly mechanical and clean pass: @Override additions, @RequiredReadAction/@RequiredWriteAction annotations, instanceof pattern binding, diamond operator, PairProcessor/Condition → BiPredicate/Predicate, SpringBundle → SpringLocalize, and 2-space → 4-space reindentation. I checked the non-trivial refactors (the merged condition in PsiAnnotatedTypePattern.acceptsAnnotationPattern, the lambda conversions in ResourceResolverUtils/PsiBeanPointcutExpression/ResolvedConstructorArgsImpl) and they preserve the original behavior. Nice work.
A few small things left inline as one-click suggestions.
One thing worth double-checking (not blocking): several call sites swap ModuleUtilCore.findModuleForPsiElement(...) / findModuleForFile(...) for xxx.getModule() (SpringManagerImpl.getLocalSpringModel, ResourceResolverUtils.getResourceFileReferenceString, PsiBeanPointcutExpression.acceptsClass). These are usually equivalent, but getModule() can resolve differently for elements in library/derived files vs. the ModuleUtilCore lookup — worth a quick confirmation that existing resolve/highlighting tests still pass.
| for (ResolveResult resolveResult : polyVariantRef.multiResolve(false)) {if (resolveResult.getElement() instanceof PsiFileSystemItem fsItem && filter.test(fsItem)) { | ||
| result.add((V) fsItem); | ||
| } | ||
| } |
There was a problem hiding this comment.
The for and the nested if got collapsed onto one line here — worth splitting for readability (and consistency with the reindentation done everywhere else in this PR).
| for (ResolveResult resolveResult : polyVariantRef.multiResolve(false)) {if (resolveResult.getElement() instanceof PsiFileSystemItem fsItem && filter.test(fsItem)) { | |
| result.add((V) fsItem); | |
| } | |
| } | |
| for (ResolveResult resolveResult : polyVariantRef.multiResolve(false)) { | |
| if (resolveResult.getElement() instanceof PsiFileSystemItem fsItem && filter.test(fsItem)) { | |
| result.add((V) fsItem); | |
| } | |
| } |
| Collections.singleton(file), | ||
| ModuleUtilCore.findModuleForPsiElement(file), | ||
| null); | ||
| return beans == null ? null : new DomSpringModelImpl2(beans, Collections.singleton(file), file.getModule(), null); |
There was a problem hiding this comment.
This return is indented one level too deep (6 spaces instead of 4).
| return beans == null ? null : new DomSpringModelImpl2(beans, Collections.singleton(file), file.getModule(), null); | |
| return beans == null ? null : new DomSpringModelImpl2(beans, Collections.singleton(file), file.getModule(), null); |
| @Override | ||
| public boolean equals(QualifierAttribute o1, QualifierAttribute o2) { | ||
| return Comparing.equal(o1.getAttributeKey(), o2.getAttributeKey()) && Comparing.equal(o1.getAttributeValue(), o2.getAttributeValue()); | ||
| return Objects.equals(o1.getAttributeKey(), o2.getAttributeKey()) && Comparing.equal(o1.getAttributeValue(), o2.getAttributeValue()); |
There was a problem hiding this comment.
Minor consistency nit: the key comparison was migrated to Objects.equals but the value comparison still uses Comparing.equal. Both are null-safe and behave the same, but using one style reads cleaner. (If you'd rather, keep both as Comparing.equal — either way, just pick one.)
| return Objects.equals(o1.getAttributeKey(), o2.getAttributeKey()) && Comparing.equal(o1.getAttributeValue(), o2.getAttributeValue()); | |
| return Objects.equals(o1.getAttributeKey(), o2.getAttributeKey()) && Objects.equals(o1.getAttributeValue(), o2.getAttributeValue()); |
…otations. Some localization and refactoring.
|
@claude review pr with inline comments |
|
Claude finished @unv-unv's task in 2m 37s —— View job PR Review — part 5 (@OverRide / RRA-RWA-RUI / localization / refactoring)
Posted a review with 2 inline comments. SummaryThis is an overwhelmingly mechanical and clean pass. I re-verified the non-trivial refactors against the actual checked-out tree and they preserve behavior:
Inline findings (both minor)
Worth double-checking (not blocking)Several sites swap No behavioral bugs found. 👍 |
There was a problem hiding this comment.
Part 5 — one more accurate pass over the non-trivial refactors. The mechanical work (@Override, RRA/RWA/RUI, instanceof patterns, diamonds, PairProcessor/Condition → BiPredicate/Predicate, SpringBundle → SpringLocalize) all preserves behavior — including the merged condition in PsiAnnotatedTypePattern.acceptsAnnotationPattern (same fall-through to the qualifiedName check) and the lambda conversions in ResourceResolverUtils / PsiBeanPointcutExpression / ResolvedConstructorArgsImpl. Two small leftovers below.
| import consulo.util.lang.Comparing; | ||
| import consulo.util.collection.HashingStrategy; | ||
|
|
||
| import consulo.util.lang.Comparing; |
There was a problem hiding this comment.
Comparing is now unused — both the key and value comparisons in equals(...) were migrated to Objects.equals, so this import can be dropped.
| import consulo.util.lang.Comparing; |
| private final ConvertContext myContext; | ||
| private final CommonSpringBean myBean; | ||
| public class PropertyReferenceSet extends ReferenceSetBase<PropertyReference> { | ||
| @Nullable |
There was a problem hiding this comment.
This file was reindented to 4 spaces, but the rest of this PR and the surrounding code (including the closely-related PropertyReference.java in the same package) use 2-space indentation. Worth reindenting back to 2 spaces for consistency.
2 participants
No description provided.