[GitHub] [ignite-3] ibessonov commented on a change in pull request #490: IGNITE-15564 Properly inject names into named list elements

[GitBox <gi...@apache.org>]

ibessonov commented on a change in pull request #490:
URL: https://github.com/apache/ignite-3/pull/490#discussion_r765509463



##########
File path: modules/configuration-annotation-processor/src/integrationTest/java/org/apache/ignite/internal/configuration/processor/ItProcessorTest.java
##########
@@ -287,21 +409,25 @@ void testStaticConstants() {
      */
     private static BatchCompilation batchCompile(String packageName, String... classNames) {
         ClassName[] classes = Arrays.stream(classNames)
-                .map(clsName -> ClassName.get(packageName, clsName))
-                .toArray(ClassName[]::new);
+            .map(clsName -> ClassName.get(packageName, clsName))

Review comment:
       Interesting, I thought that old formatting was the correct one. Can you check please?

##########
File path: modules/configuration-annotation-processor/src/integrationTest/java/org/apache/ignite/internal/configuration/processor/ItProcessorTest.java
##########
@@ -287,21 +409,25 @@ void testStaticConstants() {
      */
     private static BatchCompilation batchCompile(String packageName, String... classNames) {
         ClassName[] classes = Arrays.stream(classNames)
-                .map(clsName -> ClassName.get(packageName, clsName))
-                .toArray(ClassName[]::new);
+            .map(clsName -> ClassName.get(packageName, clsName))
+            .toArray(ClassName[]::new);
 
         return batchCompile(classes);
     }
 
-    @Test
-    void wrongSchemaPostfix() {
-        String packageName = "org.apache.ignite.internal.configuration.processor";
-
-        ClassName schema = ClassName.get(packageName, "ConfigurationSchemaWithWrongPostfix");
-
-        Compilation compilation = compile(schema);
+    /**
+     * Extends {@link Assertions#assertThrows(Class, Executable)} to check for a substring in the error message.
+     *
+     * @param expErrCls Expected error class.
+     * @param exec Supplier.
+     * @param expSubStr Expected substring in error message.
+     * @throws AssertionFailedError If failed.
+     */
+    private void assertThrowsEx(Class<? extends Throwable> expErrCls, Executable exec, @Nullable String expSubStr) {
+        Throwable t = assertThrows(expErrCls, exec);
 
-        assertThat(compilation).failed();
-        assertThat(compilation).hadErrorContaining(schema + " must end with 'ConfigurationSchema'");
+        if (expSubStr != null) {
+            assertTrue(t.getMessage().contains(expSubStr), () -> String.format("%s not contains %s", t.getMessage(), expSubStr));

Review comment:
       There's a org.hamcrest.Matchers#containsString that will save you from creating error message manually 

##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -2588,6 +2646,14 @@ private BytecodeBlock treatSourceForConstruct(
                 setField = setThisFieldCode(constructMtd, newValue, schemaFieldDef);
             }
 
+            if (isContainNameAnnotation(schemaField)) {

Review comment:
       This name just feels wrong. No one says "is contain", maybe rename it to "containsNameAnnotation"?

##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -2588,6 +2646,14 @@ private BytecodeBlock treatSourceForConstruct(
                 setField = setThisFieldCode(constructMtd, newValue, schemaFieldDef);
             }
 
+            if (isContainNameAnnotation(schemaField)) {
+                setField = new BytecodeBlock()
+                        .append(setField)
+                        .append(getThisFieldCode(constructMtd, schemaFieldDef))

Review comment:
       Why don't you use BytecodeExpressions.invoke...? It's more declarative and easier to understand

##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/InnerNode.java
##########
@@ -187,4 +188,18 @@ public InnerNode copy() {
     public <NODET> NODET specificNode() {
         return (NODET) this;
     }
+
+    /**
+     * Gets the value of a field with {@link InjectedName}.

Review comment:
       "Returns", not "Gets". Andrey Gura is very upset every time he sees the word "gets" in javadoc. Don't upset him :)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org