JBRes-7955: Implement transformation that changes file location within the project#38
JBRes-7955: Implement transformation that changes file location within the project#38Vladislav0Art wants to merge 66 commits intomainfrom
Conversation
Qodana Community for JVM2 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked View the detailed Qodana reportTo be able to view the detailed Qodana report, you can either:
To get - name: 'Qodana Scan'
uses: JetBrains/qodana-action@v2025.1.1
with:
upload-result: trueContact Qodana teamContact us at qodana-support@jetbrains.com
|
dragoi75
left a comment
There was a problem hiding this comment.
I left a few nitpicks and some things that seem broken from my limited testing.
I could not test the AI-suggested one, see my comment in the file.
Also, why do you not use the refactoring provided by the IntelliJ SDK, see MoveFilesOrDirectoriesProcessor?
I am aware your implementation probably is similar, but I think there might be edge cases that we are not thinking about, so the SDK is probably a safer option.
As a general remark, this PR is too big, which makes reviewing difficult. In the future please consider splitting in smaller PRs.
...ub/pderakhshanfar/codecocoonplugin/components/transformations/IntelliJAwareTransformation.kt
Outdated
Show resolved
Hide resolved
...in/kotlin/com/github/pderakhshanfar/codecocoonplugin/suggestions/impl/SuggestNewDirectory.kt
Outdated
Show resolved
Hide resolved
...in/kotlin/com/github/pderakhshanfar/codecocoonplugin/suggestions/impl/SuggestNewDirectory.kt
Outdated
Show resolved
Hide resolved
...hshanfar/codecocoonplugin/components/transformations/MoveFileToNewDirectoryTransformation.kt
Outdated
Show resolved
Hide resolved
...hshanfar/codecocoonplugin/components/transformations/MoveFileToNewDirectoryTransformation.kt
Outdated
Show resolved
Hide resolved
...nfar/codecocoonplugin/components/transformations/MoveFileToNewDirectoryTransformationImpl.kt
Outdated
Show resolved
Hide resolved
...nfar/codecocoonplugin/components/transformations/MoveFileToNewDirectoryTransformationImpl.kt
Outdated
Show resolved
Hide resolved
...nfar/codecocoonplugin/components/transformations/MoveFileToNewDirectoryTransformationImpl.kt
Outdated
Show resolved
Hide resolved
|
|
See `Map<String, Any>.require` and its tests in `ConfigParsingTest`.
…o-be-moved file Next steps: 1. Update imports of to-be-moved file components to match a new package. 2. Move the file.
…eFilesOrDirectoriesProcessor`
1dedfc0 to
395568b
Compare
…uggestion strategy
dragoi75
left a comment
There was a problem hiding this comment.
Overall nice :D
The comments are mostly nits, but there are two that need addressing in MoveFileIntoSuggestedDirectoryTransformation.kt. One about visibility of classes in the moved files and one about error handling.
I try to do some more manual testing.
...in/kotlin/com/github/pderakhshanfar/codecocoonplugin/suggestions/impl/SuggestNewDirectory.kt
Outdated
Show resolved
Hide resolved
...in/kotlin/com/github/pderakhshanfar/codecocoonplugin/suggestions/impl/SuggestNewDirectory.kt
Show resolved
Hide resolved
core/src/test/kotlin/com/github/pderakhshanfar/codecocoonplugin/SuggestionsTest.kt
Outdated
Show resolved
Hide resolved
core/src/test/kotlin/com/github/pderakhshanfar/codecocoonplugin/SuggestionsTest.kt
Outdated
Show resolved
Hide resolved
...hub/pderakhshanfar/codecocoonplugin/components/transformations/RenameMethodTransformation.kt
Show resolved
Hide resolved
.../codecocoonplugin/components/transformations/MoveFileIntoSuggestedDirectoryTransformation.kt
Outdated
Show resolved
Hide resolved
.../codecocoonplugin/components/transformations/MoveFileIntoSuggestedDirectoryTransformation.kt
Outdated
Show resolved
Hide resolved
.../codecocoonplugin/components/transformations/MoveFileIntoSuggestedDirectoryTransformation.kt
Outdated
Show resolved
Hide resolved
| fileToMove: PsiFile, | ||
| suggestions: List<String> | ||
| ): TransformationResult { | ||
| val filename = withReadAction { fileToMove.name } |
There was a problem hiding this comment.
If the file you are moving contains non-public classes it doesn't seem to work.
I got the following error:
✗ Failed to move file OwnerController.java java.lang.RuntimeException: `com.intellij.refactoring.BaseRefactoringProcessor$ConflictsInTestsException`: Package-local class `OwnerController` will no longer be accessible from class `org.springframework.samples.petclinic.owner.OwnerControllerTests`Claude suggested adding a method that makes the classes public. Not sure if changing them to public has other consequences, but see what it suggested below (seems to work in my testing):
/**
* Ensures all top-level classes in the Java file are public.
* This is necessary to avoid accessibility conflicts when moving files to different packages.
*/
private fun ensureClassesArePublic(project: Project, javaFile: PsiJavaFile) {
val classesToModify = withReadAction {
javaFile.classes.filter { psiClass ->
val modifierList = psiClass.modifierList
modifierList != null && !(PsiUtil.getAccessLevel(modifierList).equals(PsiUtil.ACCESS_LEVEL_PUBLIC))
}
}
if (classesToModify.isNotEmpty()) {
WriteCommandAction.runWriteCommandAction(project) {
classesToModify.forEach { psiClass ->
psiClass.modifierList?.setModifierProperty(PsiModifier.PUBLIC, true)
}
}
// Commit changes after making classes public
ApplicationManager.getApplication().invokeAndWait {
PsiDocumentManager.getInstance(project).commitAllDocuments()
}
}
}And you should call it before for (suggestion in suggestions) {.
There was a problem hiding this comment.
Actually, the problem is not in the package-local classes but when these classes are in use in another file from the same package.
Simply converting them to public class won't help because, by Java rules, only one class in a file can be public; all the rest may only be package-local.
For now, I check whether any package-local file is in use inside other files; if so, we skip this transformation: b63876e
There was a problem hiding this comment.
Fair point with the fact that only one class can be public, and yes, this does solve the issue, but limits a bit the scope of the transformation. I guess a tradeoff we have to accept.
...in/kotlin/com/github/pderakhshanfar/codecocoonplugin/suggestions/impl/SuggestNewDirectory.kt
Outdated
Show resolved
Hide resolved
dragoi75
left a comment
There was a problem hiding this comment.
Looks good, just 3 more nitpicks 🙃
|
|
||
| logger.info(" ↳ Successfully moved $filename into $suggestion!") | ||
| return TransformationResult.Success( | ||
| message = "Successfully moved $filename into $suggestion. Usage Summary:\n$usageSummary", |
There was a problem hiding this comment.
Space :) Meant to have it indented to be easier to skip when reading the logs.
| package package1; | ||
|
|
||
| public class Component2 { | ||
| // This class MUST by in the package `package2`! |
| fileToMove: PsiFile, | ||
| suggestions: List<String> | ||
| ): TransformationResult { | ||
| val filename = withReadAction { fileToMove.name } |
There was a problem hiding this comment.
Fair point with the fact that only one class can be public, and yes, this does solve the issue, but limits a bit the scope of the transformation. I guess a tradeoff we have to accept.
| if (packageLocalClassesInUseByOtherFiles(fileToMove)) { | ||
| return TransformationResult.Skipped("Cannot move $filename: Package-local classes are in use by other files") | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider also adding a filter to not move test files. Otherwise it throws an error. One way to do it can be:
val fileIndex = ProjectFileIndex.getInstance(project)
if (withReadAction {fileIndex.isInTestSourceContent(fileToMove.virtualFile)}) {
return TransformationResult.Skipped("Cannot move $filename: It is a test file")
}
Hi! Thanks! I'll address it shortly tomorrow. |
Description
Introduce a project-level transformation that moves the given file into a directory either 1) suggested by AI or 2) provided in the config as a
destinationentry.Closes: JBRes-7955
Changes
move-file-into-suggested-directory-transformation/ai)destinationentry (this transformation version is registered with ID ofmove-file-into-suggested-directory-transformation/config)The transformation uses MoveFilesOrDirectoriesProcessor.java to move a file.