Fixing compilation errors after platform localization. Some refactoring by unv-unv · Pull Request #54 · consulo/consulo-python

unv-unv · 2026-10-12T15:34:58Z

No description provided.

unv-unv · 2026-10-12T15:35:25Z

@claude review pr with inline comments

github-actions · 2026-10-12T15:35:49Z

Claude finished @unv-unv's task in 1m 46s —— View job

PR #54 review

Gather context
Review diff
Post inline comments (review)
Summary

Most of this PR is a reformat plus a switch from String to LocalizeValue in ItemPresentation.getPresentableText(), ElementDescriptionProvider and PyParameterList.getPresentableText(boolean). That migration looks correct. I found no logic bugs, and the refactorings (pattern-matching instanceof, Ref → SimpleReference, read/write action annotations) keep the behaviour the same. I did not build the project.

Inline comments (each has a one-click `suggestion` unless noted)

PyImportElementImpl.java:158: this uses LocalizeValue.localizeTODO(...) for a reference name, which is an identifier and never needs localization. It should be LocalizeValue.of(...), like the other presentations in this PR.
PyStarImportElementImpl.java:94-113: the inner if (results.isEmpty()) can never be true, and the copy loop can be new ArrayList<>(results). The suggestion also uses the pattern-matching instanceof from iterateNames().
PyStructureViewElement.java:280-288: the reformat left the three overlay = ... lines at the old indentation.
PyNames.java:209 (no suggestion): UNNAMED_ELEMENT changes type from String to LocalizeValue in the public python-psi-api, which breaks third-party code that uses it. Also, PyElementNode still has its own hardcoded "<unnamed>".
PyFunctionImpl.java:295 (nit): the line is about 150 characters; the suggestion wraps it.

Outside the diff (can't comment inline)

PyParameterList.getPresentableText(boolean) now returns LocalizeValue, but two callers still pass the result to StringBuilder.append(Object). That compiles silently and depends on LocalizeValue.toString() returning the text:

python-impl/.../projectView/PyElementNode.java:67
python-impl/.../codeInsight/imports/ImportCandidateHolder.java:142

Adding .get() at both places would make this explicit and not rely on toString(): getParameterList().getPresentableText(false).get().

Fix this →

github-actions

Inline review from Claude. Overall the migration to LocalizeValue looks right; a few points below. See the summary comment for two call sites outside the diff that are affected by the PyParameterList.getPresentableText signature change.

github-actions · 2026-10-12T15:37:30Z

-				return getRefName("<none>");
-			}
+            public LocalizeValue getPresentableText() {
+                return LocalizeValue.localizeTODO(getRefName("<none>"));


localizeTODO marks text that should be localized later, but this is a reference name (an identifier), so it never needs localization. Every other presentation in this PR uses LocalizeValue.of(...) for identifiers (PyStarImportElementImpl, PyFileImpl, etc.), and using it here too keeps the TODO markers meaningful.

Suggested change

return LocalizeValue.localizeTODO(getRefName("<none>"));

return LocalizeValue.of(getRefName("<none>"));

github-actions · 2026-10-12T15:37:30Z

+                PsiElement source = PyUtil.turnDirIntoInit(importedFile);
+                if (source instanceof PyFile) {
+                    PyFile sourceFile = (PyFile) source;
+                    PyModuleType moduleType = new PyModuleType(sourceFile);
+                    List<? extends RatedResolveResult> results = moduleType.resolveMember(
+                        name,
+                        null,
+                        AccessDirection.READ,
+                        PyResolveContext.defaultContext()
+                    );
+                    if (results != null && !results.isEmpty() && PyUtil.isStarImportableFrom(name, sourceFile)) {
+                        if (results.isEmpty()) {
+                            return Collections.emptyList();
+                        }
+                        List<RatedResolveResult> res = Lists.newArrayList();
+                        for (RatedResolveResult result : results) {
+                            res.add(result);
+                        }
+                        return res;
+                    }


Since this method is already being refactored: the inner if (results.isEmpty()) can never be true because the outer condition already checks !results.isEmpty(). The copy loop can also be a plain new ArrayList<>(results). This change would also match the pattern-matching instanceof you used in iterateNames() above. Behaviour stays the same.

Suggested change

PsiElement source = PyUtil.turnDirIntoInit(importedFile);

if (source instanceof PyFile) {

PyFile sourceFile = (PyFile) source;

PyModuleType moduleType = new PyModuleType(sourceFile);

List<? extends RatedResolveResult> results = moduleType.resolveMember(

name,

null,

AccessDirection.READ,

PyResolveContext.defaultContext()

);

if (results != null && !results.isEmpty() && PyUtil.isStarImportableFrom(name, sourceFile)) {

if (results.isEmpty()) {

return Collections.emptyList();

}

List<RatedResolveResult> res = Lists.newArrayList();

for (RatedResolveResult result : results) {

res.add(result);

}

return res;

}

if (PyUtil.turnDirIntoInit(importedFile) instanceof PyFile sourceFile) {

PyModuleType moduleType = new PyModuleType(sourceFile);

List<? extends RatedResolveResult> results = moduleType.resolveMember(

name,

null,

AccessDirection.READ,

PyResolveContext.defaultContext()

);

if (results != null && !results.isEmpty() && PyUtil.isStarImportableFrom(name, sourceFile)) {

return new ArrayList<>(results);

}

}

(You'd need to add import java.util.ArrayList;. The com.google.common.collect.Lists import then becomes unused.)

github-actions · 2026-10-12T15:37:30Z

+                    if (myVisibility == Visibility.PRIVATE || myVisibility == Visibility.PROTECTED) {
            overlay = PythonIcons.Python.Nodes.Lock;
-          }
-          else if (myVisibility == Visibility.PREDEFINED) {
+                    }
+                    else if (myVisibility == Visibility.PREDEFINED) {
            overlay = PythonIcons.Python.Nodes.Cyan_dot;
-          }
-          else if (myVisibility == Visibility.INVISIBLE) {
+                    }
+                    else if (myVisibility == Visibility.INVISIBLE) {
            overlay = PythonIcons.Python.Nodes.Red_inv_triangle;
-          }
-          if (overlay != null) {
-            iconDescriptor.addLayerIcon(overlay);
-          }
-          return iconDescriptor.toIcon();
-        }
-      }
-    };
-  }
+                    }


The reformat skipped these three overlay = ... lines, so they're still at the old indentation.

Suggested change

if (myVisibility == Visibility.PRIVATE || myVisibility == Visibility.PROTECTED) {

overlay = PythonIcons.Python.Nodes.Lock;

}

else if (myVisibility == Visibility.PREDEFINED) {

}

else if (myVisibility == Visibility.PREDEFINED) {

overlay = PythonIcons.Python.Nodes.Cyan_dot;

}

else if (myVisibility == Visibility.INVISIBLE) {

}

else if (myVisibility == Visibility.INVISIBLE) {

overlay = PythonIcons.Python.Nodes.Red_inv_triangle;

}

if (overlay != null) {

iconDescriptor.addLayerIcon(overlay);

}

return iconDescriptor.toIcon();

}

}

};

}

}

if (myVisibility == Visibility.PRIVATE || myVisibility == Visibility.PROTECTED) {

overlay = PythonIcons.Python.Nodes.Lock;

}

else if (myVisibility == Visibility.PREDEFINED) {

overlay = PythonIcons.Python.Nodes.Cyan_dot;

}

else if (myVisibility == Visibility.INVISIBLE) {

overlay = PythonIcons.Python.Nodes.Red_inv_triangle;

}

github-actions · 2026-10-12T15:37:30Z

  public static final String UNKNOWN_TYPE = "Any";

-  public static final String UNNAMED_ELEMENT = "<unnamed>";
+  public static final LocalizeValue UNNAMED_ELEMENT = LocalizeValue.localizeTODO("<unnamed>");


This changes the type of a public constant in python-psi-api from String to LocalizeValue. That's a source/binary break for any third-party code that uses PyNames.UNNAMED_ELEMENT. Inside this repo, all usages are updated. Two optional follow-ups:

PyElementNode.updateImpl (python-impl/.../projectView/PyElementNode.java:65) still hardcodes "<unnamed>". It could use PyNames.UNNAMED_ELEMENT.get() so the literal lives in one place.

If this is meant to be localized for real, consider adding a proper key to the Python localize YAML instead of localizeTODO.

github-actions · 2026-10-12T15:37:30Z

-            public String getPresentableText() {
-                return notNullize(getName(), PyNames.UNNAMED_ELEMENT) + getParameterList().getPresentableText(true);
+            public LocalizeValue getPresentableText() {
+                return LocalizeValue.join(LocalizeValue.ofNullable(getName()).orIfEmpty(PyNames.UNNAMED_ELEMENT), getParameterList().getPresentableText(true));


Nit: this line is about 150 characters. Wrapping it would match the multi-line argument style used elsewhere in this PR.

Suggested change

return LocalizeValue.join(LocalizeValue.ofNullable(getName()).orIfEmpty(PyNames.UNNAMED_ELEMENT), getParameterList().getPresentableText(true));

return LocalizeValue.join(

LocalizeValue.ofNullable(getName()).orIfEmpty(PyNames.UNNAMED_ELEMENT),

getParameterList().getPresentableText(true)

);

github-actions Bot reviewed Oct 12, 2026

View reviewed changes

Fixing compilation errors after platform localization. Some refactoring.

6749423

unv-unv force-pushed the compilation-fix branch from dc7bf45 to 6749423 Compare October 12, 2026 15:41

VISTALL merged commit cc82d85 into consulo:master Oct 12, 2026
1 check failed

unv-unv deleted the compilation-fix branch October 12, 2026 16:43

Navigation Menu

Repository navigation

Fixing compilation errors after platform localization. Some refactoring - #54

Fixing compilation errors after platform localization. Some refactoring#54
VISTALL merged 1 commit into
consulo:masterfrom
unv-unv:compilation-fix

unv-unv commented Oct 12, 2026

Uh oh!

unv-unv commented Oct 12, 2026

Uh oh!

github-actions Bot commented Oct 12, 2026 •

edited

Loading

Uh oh!

github-actions Bot left a comment

Uh oh!

github-actions Bot Oct 12, 2026

Uh oh!

github-actions Bot Oct 12, 2026

Uh oh!

github-actions Bot Oct 12, 2026

Uh oh!

github-actions Bot Oct 12, 2026

Uh oh!

github-actions Bot Oct 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	return LocalizeValue.localizeTODO(getRefName("<none>"));
	return LocalizeValue.of(getRefName("<none>"));

Conversation

unv-unv commented Oct 12, 2026

Uh oh!

unv-unv commented Oct 12, 2026

Uh oh!

github-actions Bot commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR #54 review

Inline comments (each has a one-click suggestion unless noted)

Outside the diff (can't comment inline)

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Oct 12, 2026 •

edited

Loading

Inline comments (each has a one-click `suggestion` unless noted)