Skip to content

Adding missing @Override annotations. Adding RRA/RWA/RUI annotations. Some localization and refactoring#46

Merged
VISTALL merged 1 commit into
consulo:masterfrom
unv-unv:overrides-1
Jul 22, 2026
Merged

Adding missing @Override annotations. Adding RRA/RWA/RUI annotations. Some localization and refactoring#46
VISTALL merged 1 commit into
consulo:masterfrom
unv-unv:overrides-1

Conversation

@unv-unv

@unv-unv unv-unv commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@unv-unv

unv-unv commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

@claude review pr with inline comments

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown

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


PR Review

  • Read the diff (50 files, +758/−614)
  • Check against repo conventions
  • Post inline review comments with suggestions

This is a large, mostly mechanical PR: adding @Override, @RequiredReadAction/@RequiredWriteAction/@RequiredUIAccess annotations, migrating PyBundle.message(...)PyLocalize, ApplicationManager.getApplication()Application.get(), RefSimpleReference, pattern-matching instanceof, diamond operators, and whitespace/Javadoc cleanup. The overwhelming majority is behavior-preserving and looks good. 👍

🐞 One functional bug (inline suggestion posted)

AbstractPythonTestRunConfiguration.java:370inline comment »

The instanceof PyClass pyClass pattern variable captures the old element from the outer scope, but elementRenamedOrMoved(PsiElement newElement) must read the renamed element:

protected void elementRenamedOrMoved(PsiElement newElement) {
    myClassName = pyClass.getName();          // ← writes the OLD name back
}

The original code was ((PyClass)newElement).getName(), and the PyFunction branch immediately below still correctly uses ((PyFunction)newElement).getName(). As written, renaming a test class no longer updates the run configuration. A one-click suggestion is attached inline.

Minor (non-blocking) notes

  • ImportFromExistingAction.java (~L131): @RequiredWriteAction is placed on a local Runnable runnable variable. It's harmless but unusual — the contract is normally expressed on the method/lambda body it runs in, not the variable declaration.
  • DataFrameTable.java: the private Project myProject; field remains unused (pre-existing, not introduced here) — worth deleting while the file is being touched.
  • A few files (CreateTestAction.java) end without a trailing newline.

Nothing else stood out — the localization keys, newError(...).range(...).create() annotator migration, and the instanceof pattern rewrites all preserve the original logic.
· branch overrides-1

@RequiredReadAction
protected void elementRenamedOrMoved(PsiElement newElement) {
myClassName = ((PyClass)newElement).getName();
myClassName = pyClass.getName();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: renamed class name is lost. The instanceof PyClass pyClass pattern variable captures the old element, but elementRenamedOrMoved receives the new (renamed) element in newElement. Using pyClass.getName() here writes the old name back into myClassName, so the run configuration stops tracking the class after a rename. Note the PyFunction branch just below correctly keeps ((PyFunction)newElement).getName().

Suggested change
myClassName = pyClass.getName();
myClassName = ((PyClass)newElement).getName();

@unv-unv

unv-unv commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

@claude review pr with inline comments

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown

Claude finished @unv-unv's task in 2m 24s —— View job


PR Review

  • Read the diff (50 files, +758/−614)
  • Review key logic changes (annotators, run configs, refactors)
  • Post review with corrections & notes

This is a large, mostly mechanical PR: @Override, @RequiredReadAction/@RequiredWriteAction/@RequiredUIAccess, PyBundle.message(...)PyLocalize, ApplicationManager.getApplication()Application.get(), createErrorAnnotationnewError(...).range(...).create(), pattern-matching instanceof, and diamond operators. I traced every non-trivial logic rewrite and they are behavior-preserving. 👍

⚠️ Correction to the earlier automated review

My previous pass flagged a "functional bug" at AbstractPythonTestRunConfiguration.java:370 claiming the pattern variable pyClass was written back into myClassName. That was wrong — please disregard it (and the attached inline suggestion). The actual code is:

if (element instanceof PyClass pyClass && ... && Comparing.equal(pyClass.getName(), myClassName)) {
    return new RefactoringElementAdapter() {
        @Override
        @RequiredReadAction
        protected void elementRenamedOrMoved(PsiElement newElement) {
            myClassName = ((PyClass) newElement).getName();   // ✅ reads the NEW element, correct
        }

The pyClass pattern variable is used only in the guard (checking the old element's name) — exactly as intended. elementRenamedOrMoved correctly reads newElement. The suggestion I posted was a no-op (same code, minus a space). No behavior change here; rename tracking works. Apologies for the noise.

Logic rewrites I verified as equivalent

  • ChainedComparisonsQuickFix — the nested if/else collapse into else if (myIsRightLeft) and the merged pattern-match guard in applyFix preserve the original branch structure and the isWritable() check.
  • AugmentedAssignmentQuickFix, ConvertVariadicParamIntention, DocStringTypeReference, AssignTargetAnnotatorinstanceof → pattern rewrites and the annotator migration keep the same conditions, ranges, and message keys.
  • PyUtil.showBalloon — good pattern: new LocalizeValue overload added, old String overload correctly @Deprecated + @DeprecationInfo.
  • ClientModeMultiProcessDebugger — anonymous Function → lambda is faithful.

Minor / non-blocking notes

  • ImportFromExistingAction.java (~L133): @RequiredWriteAction is placed on the local Runnable runnable variable. It's unusual — the contract normally lives on the method/lambda that executes under a write action, not the variable declaration. Worth confirming @RequiredWriteAction's @Target permits LOCAL_VARIABLE; if CI is green it compiles, but the placement reads oddly.
  • AssignTargetAnnotator.java: the getHolder().newError(...) blocks in visitPyListCompExpression / visitPyDictCompExpression / visitPySetCompExpression are indented one extra level (8 vs 6 spaces) relative to their neighbors — cosmetic only.
  • A few touched files (e.g. CreateTestAction.java) end without a trailing newline.

Nothing here blocks merge — the localization keys, annotator migration, and instanceof rewrites all preserve the original logic. The only substantive item is retracting the earlier false-positive.
· branch overrides-1

@VISTALL
VISTALL merged commit 87a6aaa into consulo:master Jul 22, 2026
1 check passed
@unv-unv
unv-unv deleted the overrides-1 branch July 22, 2026 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants