Repository navigation
Fixing compilation errors after platform localization. Some refactoring - #59
Conversation
|
@claude review pr with inline comments |
|
Claude finished @unv-unv's task in 1m 54s —— View job PR review: inline comments posted
SummaryThe PR correctly moves
I didn't compile the project because Consulo dependencies aren't available in this runner. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed by Claude. The PR correctly follows the ElementDescriptionProvider / PresentableNodeDescriptor move from String to LocalizeValue, and the InlineRuleDialog simplification (no more double wrapping) is a nice touch. All the inline notes are small:
- Correctness/robustness:
BnfDescriptionProviderconcatenates twoLocalizeValues and relies on theirtoString(); calling.get()explicitly is safer. - Localization semantics:
localizeTODOis used on dynamic or debug strings that will never become localize keys;LocalizeValue.offits better there. - Nullability: the jspecify
@NullableinLivePreviewStructureViewFactoryis the first jspecify import in the module and only resolves transitively.
I couldn't compile locally because Consulo dependencies aren't available in this runner.
| return LocalizeValue.of( | ||
| getElementDescription(psiElement, UsageViewTypeLocation.INSTANCE) + " " + | ||
| "'" + getElementDescription(psiElement, UsageViewShortNameLocation.INSTANCE) + "'" | ||
| ); |
There was a problem hiding this comment.
The recursive getElementDescription(...) calls now return LocalizeValue, so this concatenation goes through LocalizeValue.toString() without saying so. If toString() ever gives something other than the resolved text, the usage-view node text breaks without any error. I'd call .get() explicitly so the intent is clear and the result doesn't depend on toString():
| return LocalizeValue.of( | |
| getElementDescription(psiElement, UsageViewTypeLocation.INSTANCE) + " " + | |
| "'" + getElementDescription(psiElement, UsageViewShortNameLocation.INSTANCE) + "'" | |
| ); | |
| return LocalizeValue.of( | |
| getElementDescription(psiElement, UsageViewTypeLocation.INSTANCE).get() + " " + | |
| "'" + getElementDescription(psiElement, UsageViewShortNameLocation.INSTANCE).get() + "'" | |
| ); |
| if (location == UsageViewTypeLocation.INSTANCE) { | ||
| BnfRule rule = PsiTreeUtil.getParentOfType(psiElement, BnfRule.class); | ||
| return (rule == null ? "Grammar " : "Rule ") + "Attribute"; | ||
| return LocalizeValue.localizeTODO((rule == null ? "Grammar " : "Rule ") + "Attribute"); |
There was a problem hiding this comment.
Nit: building the text before passing it to localizeTODO means a later migration to real localize keys has to untangle the concatenation. Two literal strings map 1:1 onto future keys:
| return LocalizeValue.localizeTODO((rule == null ? "Grammar " : "Rule ") + "Attribute"); | |
| return rule == null ? LocalizeValue.localizeTODO("Grammar Attribute") : LocalizeValue.localizeTODO("Rule Attribute"); |
| } | ||
| String suffix = attrList.size() > 1 ? " & " + attrList.size() + " more..." : " "; | ||
| return "Attributes { " + getAttrDisplayName(firstAttr) + suffix + "}"; | ||
| return LocalizeValue.localizeTODO("Attributes { " + getAttrDisplayName(firstAttr) + suffix + "}"); |
There was a problem hiding this comment.
Nit: most of this string is user data (the attribute name and value), and the rest is formatting. localizeTODO marks text that should eventually become a localize key, but a concatenated string can't be turned into one. Use LocalizeValue.of(...), as you did for the other data-driven presentations in this method, or a parameterized localize template later.
| return LocalizeValue.localizeTODO("Attributes { " + getAttrDisplayName(firstAttr) + suffix + "}"); | |
| return LocalizeValue.of("Attributes { " + getAttrDisplayName(firstAttr) + suffix + "}"); |
| return LocalizeValue.of(file.getName()); | ||
| } | ||
| return "" + element; | ||
| return LocalizeValue.of("" + element); |
There was a problem hiding this comment.
Nit: String.valueOf is the idiomatic replacement for "" + x:
| return LocalizeValue.of("" + element); | |
| return LocalizeValue.of(String.valueOf(element)); |
|
|
||
| import jakarta.annotation.Nonnull; | ||
| import jakarta.annotation.Nullable; | ||
| import org.jspecify.annotations.Nullable; |
There was a problem hiding this comment.
This is the only org.jspecify import in the repo. grammar-kit/pom.xml and module-info.java don't declare it, so it only resolves transitively (presumably through consulo.annotation). If that's intentional, fine. Just be aware that this file now mixes jakarta.annotation.Nonnull with jspecify @Nullable. You could keep jakarta here for consistency with the rest of the module, or migrate the module to @NullMarked in a separate change.
| IElementType elementType = node != null ? node.getElementType() : null; | ||
| if (element instanceof LeafPsiElement) { | ||
| return elementType + ": '" + element.getText() + "'"; | ||
| return LocalizeValue.localizeTODO(elementType + ": '" + element.getText() + "'"); |
There was a problem hiding this comment.
This is a debug PSI dump (element type plus raw source text), so it will never be translated. localizeTODO is misleading here; LocalizeValue.of fits better. The same applies to lines 151 and 158.
| return LocalizeValue.localizeTODO(elementType + ": '" + element.getText() + "'"); | |
| return LocalizeValue.of(elementType + ": '" + element.getText() + "'"); |
| } | ||
| else if (element instanceof PsiErrorElement errorElem) { | ||
| return "PsiErrorElement: '" + errorElem.getErrorDescription() + "'"; | ||
| return LocalizeValue.localizeTODO("PsiErrorElement: '" + errorElem.getErrorDescription() + "'"); |
There was a problem hiding this comment.
Same as above: this is a debug dump, not translatable UI text.
| return LocalizeValue.localizeTODO("PsiErrorElement: '" + errorElem.getErrorDescription() + "'"); | |
| return LocalizeValue.of("PsiErrorElement: '" + errorElem.getErrorDescription() + "'"); |
| BnfFile file = (BnfFile)rule.getContainingFile(); | ||
| String className = getRulePsiClassName(rule, getPsiClassFormat(file)); | ||
| return className + ": '" + StringUtil.first(element.getText(), 30, true) + "'"; | ||
| return LocalizeValue.localizeTODO(className + ": '" + StringUtil.first(element.getText(), 30, true) + "'"); |
There was a problem hiding this comment.
Same as above.
| return LocalizeValue.localizeTODO(className + ": '" + StringUtil.first(element.getText(), 30, true) + "'"); | |
| return LocalizeValue.of(className + ": '" + StringUtil.first(element.getText(), 30, true) + "'"); |
| } | ||
| } | ||
| return elementType + ""; | ||
| return LocalizeValue.of(elementType + ""); |
There was a problem hiding this comment.
Nit:
| return LocalizeValue.of(elementType + ""); | |
| return LocalizeValue.of(String.valueOf(elementType)); |
9c53e39 to
1f2daf3
Compare
|
@claude review pr with inline comments |
|
Claude finished @unv-unv's task in 57s —— View job PR review (re-run): one new inline comment
Applied since the last review ✅
New
Still open (comments from the earlier review, not re-posted)
Everything else looks good: the I didn't compile the project or check the platform source, because neither the Consulo dependencies nor the platform repo were reachable from this runner. |
| : LocalizeValue.of(psiElement.getClass().getSimpleName()); | ||
| } | ||
| return null; | ||
| return LocalizeValue.empty(); |
There was a problem hiding this comment.
This provider is a global @ExtensionImpl, so it gets asked about every PSI element, including Java, XML and the rest, not just BNF. Before this change, null meant "not mine, ask the next provider". Please check that the migrated ElementDescriptionUtil.getElementDescription treats LocalizeValue.empty() the same way, for example with isEmpty() or == LocalizeValue.empty(), and not with a != null check. If it uses != null, this line would cut off the other providers and leave usage-view and find-usages text blank for non-BNF elements. If the new ElementDescriptionProvider contract says to return empty(), this is fine as written. I could not check the platform source from this runner.
2 participants
No description provided.