Fix PY-44858 #2775
Fix PY-44858 #2775
Conversation
| @@ -237,7 +217,7 @@ private static PsiDirectory createDirectories(Project project, String target) th | |||
| else { | |||
| subdir = resultDir.createChildDirectory(lfs, dirs[i]); | |||
| } | |||
| if (subdir.findChild(PyNames.INIT_DOT_PY) == null) { | |||
| if ((subdir.findChild(PyNames.INIT_DOT_PY) == null) && !isNamespace) { | |||
| @@ -127,6 +130,17 @@ protected void performRefactoring(final UsageInfo @NotNull [] usages) { | |||
| }), getRefactoringName(), null); | |||
| } | |||
|
|
|||
| private static boolean isInNamespacePackage(LinkedHashSet<PsiFile> myFiles) { | |||
There was a problem hiding this comment.
Judging by the name of the method, its users might expect that it checks that all files are located inside namespace packages (you can use ContainerUtil#all). I think you can safely change the method to behave this way. If it's important that only the first file is checked, it's better to explicitly indicate that in the name.
There was a problem hiding this comment.
Also, "my" prefix of variable names is reserved for class fields and, as a matter of style and code evolution, it's usually better to accept the most abstract interface (but a convenient one) still allowing you to do the job, e.g. Collection<PsiFile> instead of the concrete LinkedHashSet<PsiFile>.
| @@ -81,8 +83,9 @@ protected void performRefactoring(final UsageInfo @NotNull [] usages) { | |||
| for (UsageInfo usage : usages) { | |||
| usagesByElement.putValue(((MyUsageInfo)usage).myMovedElement, usage); | |||
| } | |||
| boolean isNamespace = isInNamespacePackage(this.mySourceFiles); | |||
There was a problem hiding this comment.
this qualifier is not necessary here.
| @@ -518,8 +521,31 @@ public static void optimizeImports(@NotNull final PsiFile file) { | |||
| PyImportOptimizer.onlyRemoveUnused().processFile(file).run(); | |||
| } | |||
|
|
|||
|
|
|||
| public static PsiFile placeFile(Project project, String path, String filename) throws IOException { | |||
There was a problem hiding this comment.
This overload of placeFile is still used in PyExtractSuperclassHelper and, therefore, in the "Extract superclass" refactoring, which still causes the same problem with unnecessary __init__.py created inside namespace packages. Please take namespace packages into account there as well, add regression tests and remove this overload entirely.
| } | ||
|
|
||
| //PY-44858 | ||
| public void testMoveNotCreateInitPyForNamespacePackagesToChildDirectory() { |
There was a problem hiding this comment.
For this kind of test scenarios you don't need namespace packages to be inside an explicit source root. So inside the test data intermediate "src" root could be removed and the test code could be simplified to:
//PY-44858
public void testMoveNotCreateInitPyForNamespacePackagesToChildDirectory() {
doMoveSymbolsTest("pkg/subpkg/A/B/module_b.py", "myfunc");
}I mean the following "flattened" hierarchy in test data:
community/python/testData/refactoring/move/moveNotCreateInitPyForNamespacePackagesToChildDirectory/
├── after
│ └── src
│ └── pkg
│ └── subpkg
│ └── A
│ ├── B
│ │ └── module_b.py
│ └── module_a.py
└── before
└── src
└── pkg
└── subpkg
└── A
└── module_a.py
| @@ -89,7 +89,7 @@ protected final void performRefactoring(UsageInfo @NotNull [] usages) { | |||
|
|
|||
| assert ApplicationManager.getApplication().isWriteAccessAllowed(); | |||
|
|
|||
There was a problem hiding this comment.
Same problem as with "Extract superclass". This processor is responsible for refactorings moving a method or a local function to the top-level of a file (triggered by the normal Move action). If the original function is inside a namespace package and is going to be moved to a different file, unnecessary __init__.py will be created. Please add tests on that (ideally both for "Make method top-level" and "Make local function top-level").
| @@ -241,6 +244,16 @@ private boolean isFromEnclosingScope(@NotNull PsiElement element) { | |||
| !(ScopeUtil.getScopeOwner(element) instanceof PsiFile); | |||
| } | |||
|
|
|||
| private static boolean isFunctionInNamespacePackage(PyFunction myFunction){ | |||
There was a problem hiding this comment.
here need @NotNull check for PyFunction myFunction
Added method
isInNamespacePackageand variableisNamespaceto classes in "Move", that used for separation between two cases (Namespace and ordinary python package)