Repository navigation
Localizing ClassPresentationUtil - #312
Conversation
|
@claude review pr with inline comments |
|
Claude finished @unv-unv's task in 1m 54s —— View job PR review: Localizing ClassPresentationUtil
Moving I posted 6 inline comments. Most include one-click suggestions. Should fix
Nits
I didn't build the project, so I haven't confirmed that the |
| StringBuilder buffer = new StringBuilder(128); | ||
| PsiClass containingClass = myMethod.getContainingClass(); | ||
| if (containingClass != null) { | ||
| buffer.append(ClassPresentationUtil.getNameForClass(containingClass, false)); |
There was a problem hiding this comment.
getNameForClass now returns LocalizeValue, so this compiles via StringBuilder.append(Object) and depends on LocalizeValue.toString(). Every other call site in this PR uses .get() explicitly. It's safer and more consistent to do the same here:
| buffer.append(ClassPresentationUtil.getNameForClass(containingClass, false)); | |
| buffer.append(ClassPresentationUtil.getNameForClass(containingClass, false).get()); |
| } | ||
|
|
||
| @RequiredReadAction | ||
| private String getElementPresentableName(T element) { |
There was a problem hiding this comment.
Same issue at line 250, which is just outside the diff so I can't attach a suggestion to it. return ClassPresentationUtil.getNameForClass(aClass, false) + "." + element.getName(); now concatenates a LocalizeValue and so depends on its implicit toString(). Use .get() explicitly, like the other call sites:
return ClassPresentationUtil.getNameForClass(aClass, false).get() + "." + element.getName();| } | ||
| LocalizeValue name = getNameForElement(parent, qualified); | ||
| if (name.isNotEmpty()) { | ||
| return name.getNullIfEmpty(); |
There was a problem hiding this comment.
Nit: the isNotEmpty() check already passed, so getNullIfEmpty() can't return null here. get() is clearer:
| return name.getNullIfEmpty(); | |
| return name.get(); |
| } | ||
| @RequiredReadAction | ||
| public static LocalizeValue getFunctionalExpressionPresentation(PsiFunctionalExpression functionalExpression, boolean qualified) { | ||
| return LocalizeValue.localizeTODO("Functional expression in " + getContextName(functionalExpression, qualified)); |
There was a problem hiding this comment.
Nit: localizeTODO with a concatenated string bakes the context name into the untranslated text. Leaving this as a TODO is fine. When a key is added, it should take the context name as an argument (e.g. functional.expression.in.context, with getContextName passed through StringUtil.notNullize) so translators get one stable template. A small related point: if getContextName returns null, this still renders "Functional expression in null". That's how it behaved before this PR too.
| if (element instanceof PsiField) { | ||
| Module module = ModuleUtilCore.findModuleForPsiElement((PsiField) element); |
There was a problem hiding this comment.
Consistency: PsiClassFavoriteNodeProvider and PsiMethodFavoriteNodeProvider were switched to getModule() in this PR, but this one still uses ModuleUtilCore. Suggest aligning them (the ModuleUtilCore import can then be dropped):
| if (element instanceof PsiField) { | |
| Module module = ModuleUtilCore.findModuleForPsiElement((PsiField) element); | |
| if (element instanceof PsiField field) { | |
| Module module = field.getModule(); |
| PsiFile file = psiClass.getContainingFile(); | ||
| if (file instanceof PsiClassOwner classOwner) { | ||
| String packageName = classOwner.getPackageName(); | ||
| if (packageName.length() == 0) { |
There was a problem hiding this comment.
Nit: since this block is being reformatted anyway, use isEmpty(). PsiClassListCellRenderer.getContainerTextStatic does the same thing and already uses it:
| if (packageName.length() == 0) { | |
| if (packageName.isEmpty()) { |
a79253d to
513d146
Compare
|
@claude review pr with inline comments |
|
Claude finished @unv-unv's task in 58s —— View job PR review: Localizing ClassPresentationUtil
The earlier inline comments no longer match the code. The current head already contains five of the six fixes, so I didn't post them again:
Those five threads are outdated and can be resolved. All 22 call sites of
Still open (nit, no new inline comment):
The existing inline thread on that line covers both points. No new problems found. I didn't post any new inline comments because the diff has nothing new to fix. I didn't build the project, so I haven't checked that the |
No description provided.