Splitting intellij module into 3 modules: core, java-psi, kotlin-psi #22

anchouls · 2023-08-29T19:40:04Z

No description provided.

onewhl

Thank you! I have several comments.
1). I think the structure of the main and test folders should be consistent. For example, there is an inconsistency in the kotlin-psi module: org/jetbrains/academy/test/kotlin and org/jetbrains/academy/test/system/ij/kotlin. In one case you use system and in another you don't.

2). It's not needed to add .gitignore to each module, we need one .gitingore file in the root folder.
3). I suggest renaming ij.core to ij.common, but it's a very minor thing.
Please, let me know what you think about.

anchouls · 2023-08-30T11:21:41Z

@onewhl Thank you! About item (2) - the formatting check is the same for both Java and Kotlin, but the tests are different for them, that's why I defined it that way. But I can write tests for Java and Kotlin for the formatting function in a common module.

onewhl · 2023-08-30T11:24:53Z

@anchouls yes, I get it, you're absolutely right, I already removed the comment.

onewhl

Looks good to me, thanks!
I'm only not sure if we need to update intellij version to 1.15.0 in this PR, but if it breaks nothing, maybe it's ok.

nbirillo

Also, please bump the framework version. I also committed a small change of the main gradle, hope you don't mind

ij/common/build.gradle.kts

nbirillo · 2023-08-31T12:26:21Z

...i/src/main/kotlin/org/jetbrains/academy/test/system/kotlin/ij/analyzer/IjCodeAnalyzerUtil.kt

@@ -1,4 +1,4 @@
-package org.jetbrains.academy.test.system.ij.analyzer
+package org.jetbrains.academy.test.system.kotlin.ij.analyzer


I guess this file has to be moved into the common module since it works with PSI elements, not with KtElements

Why? It works with KtElements

Because now you have a lot of the same methods in Java and Kotlin modules, e.g.

fun PsiFile.hasClass(className: String): Boolean = hasElementOfTypeWithName(PsiClass::class.java, className

and

fun PsiFile.hasClass(className: String): Boolean = hasElementOfTypeWithName(KtClass::class.java, className)

Instead it you can move these methods in the ij core module and here just call them with necessary arguments

build.gradle.kts

ij/common/src/main/kotlin/org/jetbrains/academy/test/system/inspections/InspectionsUtil.kt

...psi/src/main/kotlin/org/jetbrains/academy/test/system/java/ij/analyzer/IjCodeAnalyzerUtil.kt

...i/src/main/kotlin/org/jetbrains/academy/test/system/kotlin/ij/analyzer/IjCodeAnalyzerUtil.kt

nbirillo

Thank you! I have several suggestions

nbirillo · 2023-12-13T11:45:56Z

...psi/src/main/kotlin/org/jetbrains/academy/test/system/java/ij/analyzer/IjCodeAnalyzerUtil.kt

@@ -0,0 +1,196 @@
+package org.jetbrains.academy.test.system.java.ij.analyzer


I'd suggest to separate this file into several ones, since it is loo long now, e.g.:

PsiFileAnalyzerUtil
IjCodeAnalyzerUtil
PsiElementAnalyzerUtil

What do you think?

nbirillo · 2023-12-13T11:48:57Z

ij/java-psi/src/test/kotlin/org/jetbrains/academy/test/system/java/ij/BaseIjTestClassTests.kt

+        var methodName = "method(\"Content\")"
+        var methodsList = listOf("outerFunction", "method2")
+        assert(methodsList.equals(findMethodUsages(methodName))) {
+            "Method $methodName should be called in methods: $methodsList"
+        }
+        methodName = "method(\"y is greater than 5\")"
+        methodsList = listOf("method1")
+        assert(methodsList.equals(findMethodUsages(methodName))) {
+            "Method $methodName should be called in methods: $methodsList"
+        }
+        methodName = "method(\"y is less than 5\")"
+        methodsList = listOf("method1")
+        assert(methodsList.equals(findMethodUsages(methodName))) {
+            "Method $methodName should be called in methods: $methodsList"
+        }
+        methodName = "method(content)"
+        methodsList = listOf()
+        assert(methodsList.equals(findMethodUsages(methodName))) {
+            "Method $methodName should not be called"
+        }


Should we change this test into a parametrized one?

nbirillo · 2023-12-13T11:49:37Z

ij/java-psi/src/test/kotlin/org/jetbrains/academy/test/system/java/ij/BaseIjTestClassTests.kt

+
+class BaseIjTestClassTests : BaseIjTestClass() {
+
+    fun testFindMethodsWithContent() {


We can also make this test as a parametrized one

Or at least to move common functionality and call a function here

nbirillo · 2023-12-13T11:50:22Z

ij/java-psi/src/test/kotlin/org/jetbrains/academy/test/system/java/ij/BaseIjTestClassTests.kt

+            }
+        """.trimIndent()
+        myFixture.configureByText("Task.java", example)
+        var expression = "productPrice.size()"


nbirillo · 2023-12-13T11:51:46Z

...i/src/main/kotlin/org/jetbrains/academy/test/system/kotlin/ij/analyzer/IjCodeAnalyzerUtil.kt

@@ -1,4 +1,4 @@
-package org.jetbrains.academy.test.system.ij.analyzer
+package org.jetbrains.academy.test.system.kotlin.ij.analyzer


Because now you have a lot of the same methods in Java and Kotlin modules, e.g.

fun PsiFile.hasClass(className: String): Boolean = hasElementOfTypeWithName(PsiClass::class.java, className

and

fun PsiFile.hasClass(className: String): Boolean = hasElementOfTypeWithName(KtClass::class.java, className)

Instead it you can move these methods in the ij core module and here just call them with necessary arguments

…rameterized tests

nbirillo

Thank you!

anchouls requested review from nbirillo and onewhl August 29, 2023 19:40

onewhl requested changes Aug 30, 2023

View reviewed changes

onewhl approved these changes Aug 31, 2023

View reviewed changes

nbirillo requested changes Aug 31, 2023

View reviewed changes

anchouls force-pushed the intellij-java-module branch from 92fca18 to 4c72002 Compare December 11, 2023 13:48

anchouls and others added 8 commits December 11, 2023 15:02

Added 3 modules: core, java-psi, kotlin-psi

32348d9

Renamed the common module, packages and removed gitignore

001452c

Updated package

303f47f

Added java-psi implementation

99cc83c

Update gradle

a9331ec

Implementation of the function to optimize imports for java code

6afa4f6

Fix publishing

40bc8fb

Fixed formatting util

56ca009

anchouls force-pushed the intellij-java-module branch from 4c72002 to 56ca009 Compare December 11, 2023 14:20

Added methods to check whether elements of the specified type exist

5556d8a

anchouls requested review from nbirillo and onewhl December 11, 2023 16:43

onewhl reviewed Dec 11, 2023

View reviewed changes

anchouls added 2 commits December 11, 2023 18:40

Deleted duplicate comments

69b8331

Removed the check for Global Inspections

219755e

onewhl approved these changes Dec 12, 2023

View reviewed changes

nbirillo requested changes Dec 13, 2023

View reviewed changes

Moved some functions from PsiAnalyzerUtil to the common module and pa…

94dce6d

…rameterized tests

anchouls requested a review from nbirillo December 13, 2023 14:29

anchouls force-pushed the intellij-java-module branch from c85157d to 94dce6d Compare December 15, 2023 12:04

nbirillo approved these changes Dec 15, 2023

View reviewed changes

nbirillo merged commit 282faa4 into main Dec 15, 2023
4 checks passed

nbirillo deleted the intellij-java-module branch December 15, 2023 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Splitting intellij module into 3 modules: core, java-psi, kotlin-psi #22

Splitting intellij module into 3 modules: core, java-psi, kotlin-psi #22

anchouls commented Aug 29, 2023

onewhl left a comment •

edited

Loading

anchouls commented Aug 30, 2023

onewhl commented Aug 30, 2023

onewhl left a comment

nbirillo left a comment

nbirillo Aug 31, 2023

anchouls Aug 31, 2023

nbirillo Dec 13, 2023

nbirillo left a comment

nbirillo Dec 13, 2023

nbirillo Dec 13, 2023

nbirillo Dec 13, 2023

nbirillo Dec 13, 2023

nbirillo Dec 13, 2023

nbirillo Dec 13, 2023

nbirillo left a comment

		@@ -1,4 +1,4 @@
		package org.jetbrains.academy.test.system.ij.analyzer
		package org.jetbrains.academy.test.system.kotlin.ij.analyzer

		@@ -0,0 +1,196 @@
		package org.jetbrains.academy.test.system.java.ij.analyzer


		class BaseIjTestClassTests : BaseIjTestClass() {

		fun testFindMethodsWithContent() {

Splitting intellij module into 3 modules: core, java-psi, kotlin-psi #22

Splitting intellij module into 3 modules: core, java-psi, kotlin-psi #22

Conversation

anchouls commented Aug 29, 2023

onewhl left a comment • edited Loading

Choose a reason for hiding this comment

anchouls commented Aug 30, 2023

onewhl commented Aug 30, 2023

onewhl left a comment

Choose a reason for hiding this comment

nbirillo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbirillo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbirillo left a comment

Choose a reason for hiding this comment

onewhl left a comment •

edited

Loading