You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2021/03/23 16:51:16 UTC

[GitHub] [sling-org-apache-sling-repoinit-parser] anchela opened a new pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

anchela opened a new pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9


   @bdelacretaz , please find an initial POC for that improvement. i use the code from https://github.com/apache/sling-org-apache-sling-feature-cpconverter/pull/67 and spotted a few missing pieces :). but there likely are more..... whatever i felt could be problematic i flagged with FIXME and TODO.
   wdyt?


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] bdelacretaz commented on pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#issuecomment-807115350


   You should see my comment ("I think this doesn't work for SetAclPrincipalBased ") near the bottom of https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9/files
   
   The tests still pass with the below change, for example, because Operations are compared using `toString()` which does not necessarily include all parameters from the input.
   
       diff --git a/src/main/java/org/apache/sling/repoinit/parser/operations/SetAclPrincipalBased.java b/src/main/java/org/apache/sling/repoinit/parser/operations/SetAclPrincipalBased.java
       index c8d8538..aae54ef 100644
       --- a/src/main/java/org/apache/sling/repoinit/parser/operations/SetAclPrincipalBased.java
       +++ b/src/main/java/org/apache/sling/repoinit/parser/operations/SetAclPrincipalBased.java
       @@ -51,7 +51,7 @@ public class SetAclPrincipalBased extends AclGroupBase {
            @NotNull
            @Override
            public String asRepoInitString() {
       -        String topline = String.format("set principal ACL for %s%s%n", listToString(principals), getAclOptionsString());
       +        String topline = String.format("set principal ACL for %s%s%n", listToString(principals), "");
                return asRepoInit(topline, true);
            }
   


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] bdelacretaz commented on pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#issuecomment-808073009


   Hi @anchela , I don't know why you're not seeing my comment, here's a screenshot, let me know if you understand why it's not visible to you, maybe I did something wrong?
   
   ![my-comment](https://user-images.githubusercontent.com/81101/112611617-591a7000-8e1e-11eb-815e-b2fde72f5c73.jpg)
   
   The scenario that I'm suggesting to test this "reconstruction of repoinit statements":
   
   * Start with one of the existing test cases as currently done in `AsRepoInitTest`
   * Parse the input script E
   * Walk the list of `Operation` returned and reconstruct a script A using `asRepoInitString()`
   * Compare E and A a Strings
   
   As is, the comparison will probably fail due to A not including comments and maybe whitespace differences.
   
   Normalizing both E and A by removing comments and fixing whitespace should allow for comparing them?
   
   I think that's the only way to reliably test this, my `topline` patch above shows that currently some wrong `asRepoInitString()` implementations won't cause the tests to fail.


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] anchela commented on pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
anchela commented on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#issuecomment-810058711


   @bdelacretaz , did you have a chance to take another look? i would appreciate if we could resolve this.....


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] sonarcloud[bot] removed a comment on pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#issuecomment-805973760


   SonarCloud Quality Gate failed.
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=BUG)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=VULNERABILITY)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=CODE_SMELL)
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='93.1%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&metric=new_coverage&view=list) [93.1% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&metric=new_coverage&view=list)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/5.png' alt='3.9%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&metric=new_duplicated_lines_density&view=list) [3.9% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] anchela commented on a change in pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
anchela commented on a change in pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#discussion_r600431559



##########
File path: src/main/java/org/apache/sling/repoinit/parser/operations/Operation.java
##########
@@ -41,4 +46,20 @@ public static String cleanupQuotedString(String s) {
         }
         return s;
     }
+
+    @NotNull
+    static String escape(@NotNull String s) {

Review comment:
       yes, you are right




-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] anchela commented on pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
anchela commented on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#issuecomment-806992693


   @bdelacretaz , i am sorry.... but don't find your comment. is the test failing for you? or what is the problem you are seeing?


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] bdelacretaz commented on a change in pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on a change in pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#discussion_r602295407



##########
File path: src/test/java/org/apache/sling/repoinit/parser/test/AsRepoInitTest.java
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.sling.repoinit.parser.test;
+
+import org.apache.sling.repoinit.parser.RepoInitParsingException;
+import org.apache.sling.repoinit.parser.impl.RepoInitParserService;
+import org.apache.sling.repoinit.parser.operations.Operation;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+import java.io.IOException;
+import java.io.StringReader;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+/** Test the parser using our test-* input/expected output files.
+ *  The code of this class doesn't contain any actual tests, it
+ *  just looks for test-*.txt files, parses them and verifies the
+ *  results according to the test-*-output.txt files.
+ */
+@RunWith(Parameterized.class)
+public class AsRepoInitTest {
+
+    private final ParserTest.TestCase tc;
+
+    @Parameters(name="{0}")
+    public static Collection<Object[]> data() throws IOException {
+        final List<Object []> result = new ArrayList<>();
+        for(int i=0; i < 100; i++) {
+            final ParserTest.TestCase tc = ParserTest.TestCase.build(i);
+            if(tc != null) {
+                result.add(new Object[] { tc });
+            }
+        }
+        return result;
+
+    }
+
+    public AsRepoInitTest(ParserTest.TestCase tc) {
+        this.tc = tc;
+    }
+
+    @Test
+    public void checkResultAsRepoInit() throws Exception {
+        StringBuilder repoInit = new StringBuilder();
+        final List<Operation> expectedResult;
+        try {
+            expectedResult = new RepoInitParserService().parse(tc.input);
+            for (Operation o : expectedResult) {
+                repoInit.append(o.asRepoInitString());
+            }
+        } finally {
+            tc.close();
+        }
+
+        try {
+            List<Operation> ops = new RepoInitParserService().parse(new StringReader(repoInit.toString()));
+            assertEquals(expectedResult.size(), ops.size());
+            for (int i = 0; i < expectedResult.size(); i++) {
+                Operation expected = expectedResult.get(i);
+                Operation op = ops.get(i);
+                assertEquals(expected.toString(), op.toString());

Review comment:
       Making `getParametersDescription` public, or rather part of the `Operation` interface, works for me, and its javadoc should then indicate that it must take _all_ parameters into account.




-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] bdelacretaz edited a comment on pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
bdelacretaz edited a comment on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#issuecomment-808073009


   _edit: in the meantime Nicolas Peltier explained why my comment wasn't visible at https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9/files, I have submitted my review now and that should be fixed)_
   
   The scenario that I'm suggesting to test this "reconstruction of repoinit statements":
   
   * Start with one of the existing test cases as currently done in `AsRepoInitTest`
   * Parse the input script E
   * Walk the list of `Operation` returned and reconstruct a script A using `asRepoInitString()`
   * Compare E and A a Strings
   
   As is, the comparison will probably fail due to A not including comments and maybe whitespace differences.
   
   Normalizing both E and A by removing comments and fixing whitespace should allow for comparing them?
   
   I think that's the only way to reliably test this, my `topline` patch above shows that currently some wrong `asRepoInitString()` implementations won't cause the tests to fail.


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] anchela commented on pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
anchela commented on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#issuecomment-805977433


   @bdelacretaz from my side this PR is complete. the remaining fixme can currently not be resolved as the operation no longer has that information. however, as far as I can see it doesn't change the outcome when applying the generated repo-init.... it's more about generating the exact same statements.
   wdyt?
   would you have time for a another review and eventually merge it? would appreciate it.
   and big thanks to @karlpauls who spotted the reason for the failing test!


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] anchela commented on pull request #9: SLING-10236 : Introduce Operation.asRepoInitString

Posted by GitBox <gi...@apache.org>.
anchela commented on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#issuecomment-810140246


   @bdelacretaz , LGTM


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] anchela commented on pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
anchela commented on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#issuecomment-808401095


   @bdelacretaz , moving the test-case from parser.test package to parser.operations and make a few things in ParserTest.TestCase public did the trick and allowed to access the Operation.getParameterDescription and using it instead of Operation.toString in the test case. the build passed locally for me..... wdty?


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] bdelacretaz commented on a change in pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on a change in pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#discussion_r600335701



##########
File path: src/main/java/org/apache/sling/repoinit/parser/operations/Operation.java
##########
@@ -41,4 +46,20 @@ public static String cleanupQuotedString(String s) {
         }
         return s;
     }
+
+    @NotNull
+    static String escape(@NotNull String s) {

Review comment:
       escapeQuotes might be good as a more specific method name?




-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] sonarcloud[bot] commented on pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#issuecomment-805075270


   Kudos, SonarCloud Quality Gate passed!
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=BUG)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=VULNERABILITY)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=CODE_SMELL) [5 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=CODE_SMELL)
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='93.1%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&metric=new_coverage&view=list) [93.1% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&metric=new_coverage&view=list)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='2.7%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&metric=new_duplicated_lines_density&view=list) [2.7% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] anchela commented on a change in pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
anchela commented on a change in pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#discussion_r602235934



##########
File path: src/test/java/org/apache/sling/repoinit/parser/test/AsRepoInitTest.java
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.sling.repoinit.parser.test;
+
+import org.apache.sling.repoinit.parser.RepoInitParsingException;
+import org.apache.sling.repoinit.parser.impl.RepoInitParserService;
+import org.apache.sling.repoinit.parser.operations.Operation;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+import java.io.IOException;
+import java.io.StringReader;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+/** Test the parser using our test-* input/expected output files.
+ *  The code of this class doesn't contain any actual tests, it
+ *  just looks for test-*.txt files, parses them and verifies the
+ *  results according to the test-*-output.txt files.
+ */
+@RunWith(Parameterized.class)
+public class AsRepoInitTest {
+
+    private final ParserTest.TestCase tc;
+
+    @Parameters(name="{0}")
+    public static Collection<Object[]> data() throws IOException {
+        final List<Object []> result = new ArrayList<>();
+        for(int i=0; i < 100; i++) {
+            final ParserTest.TestCase tc = ParserTest.TestCase.build(i);
+            if(tc != null) {
+                result.add(new Object[] { tc });
+            }
+        }
+        return result;
+
+    }
+
+    public AsRepoInitTest(ParserTest.TestCase tc) {
+        this.tc = tc;
+    }
+
+    @Test
+    public void checkResultAsRepoInit() throws Exception {
+        StringBuilder repoInit = new StringBuilder();
+        final List<Operation> expectedResult;
+        try {
+            expectedResult = new RepoInitParserService().parse(tc.input);
+            for (Operation o : expectedResult) {
+                repoInit.append(o.asRepoInitString());
+            }
+        } finally {
+            tc.close();
+        }
+
+        try {
+            List<Operation> ops = new RepoInitParserService().parse(new StringReader(repoInit.toString()));
+            assertEquals(expectedResult.size(), ops.size());
+            for (int i = 0; i < expectedResult.size(); i++) {
+                Operation expected = expectedResult.get(i);
+                Operation op = ops.get(i);
+                assertEquals(expected.toString(), op.toString());

Review comment:
       @bdelacretaz , comparing repoInit.tostring with tc.input won't work for the following reasons:
   - 1 operation doesn't allow to generate the original string (see FIXME with SLING-jira-reference)
   - instructions like create service user a,b,c get converted to 3x CreateServiceUserOperation and it is impossible to know if that was 3x create service user or the shortcut.
   
   the only way to do that is manually creating a result.txt.... but that then probably generates a lot of extra maintenance effort for you and there is a tiny risk of the result.txt also containing flaws.
   
   what might be more reliable than toString is getParameterDescription.... for some ops it is public. but not for all..... but i was a bit hesitant to make it public just for testing purpose in the light of an issue that is otherwise unrelated. but if you make it public i would use it instead of toString. wdyt?




-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] bdelacretaz commented on pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#issuecomment-806493579


   I still see a problem with `assertEquals(expected.toString(), op.toString())` in AsRepoinitTest, I have commented there.


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] anchela edited a comment on pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
anchela edited a comment on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#issuecomment-807999041


   @bdelacretaz , in my original draft in the cp-fm-converter i compared original, expected repo-init with the generated out and  verified that SetAclPrincipalBased works. what exactly do you expect not working for the SetAclPrincipalBased?
   I already noted in the JIRA ticket that toString is not implemented in a consistent way. but unfortunately the getParameterDescription method is not public (and also not consistently implemented either).... so {{toString]} was the best present today..... what would you suggest?
   
   and btw: i was searching in  https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9/files before and again now.... but i don't see any comment (even another pair of eyes looking at it at my screen ;-)).


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] anchela edited a comment on pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
anchela edited a comment on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#issuecomment-807999041


   @bdelacretaz , in my original draft in the cp-fm-converter i compared original, expected repo-init with the generated out and  verified that SetAclPrincipalBased works. what exactly do you expect not working for the SetAclPrincipalBased?
   I already noted in the JIRA ticket that toString is not implemented in a consistent way. but unfortunately the getParameterDescription method is not public (and also not consistently implemented).... so {{toString]} was the best present today..... what would you suggest?
   
   and btw: i was searching in  https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9/files before and again now.... but i don't see any comment (even another pair of eyes looking at it at my screen ;-)).


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] anchela commented on a change in pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
anchela commented on a change in pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#discussion_r602235934



##########
File path: src/test/java/org/apache/sling/repoinit/parser/test/AsRepoInitTest.java
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.sling.repoinit.parser.test;
+
+import org.apache.sling.repoinit.parser.RepoInitParsingException;
+import org.apache.sling.repoinit.parser.impl.RepoInitParserService;
+import org.apache.sling.repoinit.parser.operations.Operation;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+import java.io.IOException;
+import java.io.StringReader;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+/** Test the parser using our test-* input/expected output files.
+ *  The code of this class doesn't contain any actual tests, it
+ *  just looks for test-*.txt files, parses them and verifies the
+ *  results according to the test-*-output.txt files.
+ */
+@RunWith(Parameterized.class)
+public class AsRepoInitTest {
+
+    private final ParserTest.TestCase tc;
+
+    @Parameters(name="{0}")
+    public static Collection<Object[]> data() throws IOException {
+        final List<Object []> result = new ArrayList<>();
+        for(int i=0; i < 100; i++) {
+            final ParserTest.TestCase tc = ParserTest.TestCase.build(i);
+            if(tc != null) {
+                result.add(new Object[] { tc });
+            }
+        }
+        return result;
+
+    }
+
+    public AsRepoInitTest(ParserTest.TestCase tc) {
+        this.tc = tc;
+    }
+
+    @Test
+    public void checkResultAsRepoInit() throws Exception {
+        StringBuilder repoInit = new StringBuilder();
+        final List<Operation> expectedResult;
+        try {
+            expectedResult = new RepoInitParserService().parse(tc.input);
+            for (Operation o : expectedResult) {
+                repoInit.append(o.asRepoInitString());
+            }
+        } finally {
+            tc.close();
+        }
+
+        try {
+            List<Operation> ops = new RepoInitParserService().parse(new StringReader(repoInit.toString()));
+            assertEquals(expectedResult.size(), ops.size());
+            for (int i = 0; i < expectedResult.size(); i++) {
+                Operation expected = expectedResult.get(i);
+                Operation op = ops.get(i);
+                assertEquals(expected.toString(), op.toString());

Review comment:
       @bdelacretaz , comparing repoInit.tostring with tc.input won't work for the following reasons:
   - 1 operation doesn't allow to generate the original string (see FIXME with SLING-jira-reference)
   - instructions like create service user a,b,c get converted to 3x CreateServiceUserOperation and it is impossible to know if that was 3x create service user or the shortcut.
   the only way to do that is manually creating a result.txt.... but that then probably generates a lot of extra maintenance effort for you and there is a tiny risk of the result.txt also containing flaws.
   
   what might be more reliable than toString is getParameterDescription.... for some ops it is public. but not for all..... but i was a bit hesitant to make it public just for testing purpose in the light of an issue that is otherwise unrelated. but if you make it public i would use it instead of toString. wdyt?




-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] anchela commented on pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
anchela commented on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#issuecomment-805949919


   hi @bdelacretaz could it be that your commit for https://github.com/apache/sling-org-apache-sling-repoinit-parser/commit/44a75b0408cc3a55087c5443ee6eff3a2d6816a9 introduced new parser features that broken the improvement suggested here? if that was the case, this would be a perfect example why "Operation.asRepoInit" should reside as close to the parser as possible :) wdyt?


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] sonarcloud[bot] commented on pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#issuecomment-808401139


   SonarCloud Quality Gate failed.
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=BUG)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=VULNERABILITY)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=CODE_SMELL)
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='93.1%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&metric=new_coverage&view=list) [93.1% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&metric=new_coverage&view=list)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/5.png' alt='3.8%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&metric=new_duplicated_lines_density&view=list) [3.8% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] bdelacretaz commented on a change in pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on a change in pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#discussion_r600723588



##########
File path: src/test/java/org/apache/sling/repoinit/parser/test/AsRepoInitTest.java
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.sling.repoinit.parser.test;
+
+import org.apache.sling.repoinit.parser.RepoInitParsingException;
+import org.apache.sling.repoinit.parser.impl.RepoInitParserService;
+import org.apache.sling.repoinit.parser.operations.Operation;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+import java.io.IOException;
+import java.io.StringReader;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
+/** Test the parser using our test-* input/expected output files.
+ *  The code of this class doesn't contain any actual tests, it
+ *  just looks for test-*.txt files, parses them and verifies the
+ *  results according to the test-*-output.txt files.
+ */
+@RunWith(Parameterized.class)
+public class AsRepoInitTest {
+
+    private final ParserTest.TestCase tc;
+
+    @Parameters(name="{0}")
+    public static Collection<Object[]> data() throws IOException {
+        final List<Object []> result = new ArrayList<>();
+        for(int i=0; i < 100; i++) {
+            final ParserTest.TestCase tc = ParserTest.TestCase.build(i);
+            if(tc != null) {
+                result.add(new Object[] { tc });
+            }
+        }
+        return result;
+
+    }
+
+    public AsRepoInitTest(ParserTest.TestCase tc) {
+        this.tc = tc;
+    }
+
+    @Test
+    public void checkResultAsRepoInit() throws Exception {
+        StringBuilder repoInit = new StringBuilder();
+        final List<Operation> expectedResult;
+        try {
+            expectedResult = new RepoInitParserService().parse(tc.input);
+            for (Operation o : expectedResult) {
+                repoInit.append(o.asRepoInitString());
+            }
+        } finally {
+            tc.close();
+        }
+
+        try {
+            List<Operation> ops = new RepoInitParserService().parse(new StringReader(repoInit.toString()));
+            assertEquals(expectedResult.size(), ops.size());
+            for (int i = 0; i < expectedResult.size(); i++) {
+                Operation expected = expectedResult.get(i);
+                Operation op = ops.get(i);
+                assertEquals(expected.toString(), op.toString());

Review comment:
       I think this doesn't work for `SetAclPrincipalBased` and maybe other `Operation` classes where toString() doesn't include all the parameters. We might define that toString must include all parameters but that's hard to enforce. I think comparing `repoInit.toString()` with the contents of `tc.input` would test what we actually want to test, but the comparison probably requires normalizing both strings, at least in terms of whitespace, and maybe tweaking the test scripts if that helps getting equal values once normalized.

##########
File path: src/test/java/org/apache/sling/repoinit/parser/test/AsRepoInitTest.java
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.sling.repoinit.parser.test;
+
+import org.apache.sling.repoinit.parser.impl.RepoInitParserService;
+import org.apache.sling.repoinit.parser.operations.Operation;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+import java.io.IOException;
+import java.io.StringReader;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import static org.junit.Assert.assertEquals;
+
+/** Test the parser using our test-* input/expected output files.
+ *  The code of this class doesn't contain any actual tests, it
+ *  just looks for test-*.txt files, parses them and verifies the
+ *  results according to the test-*-output.txt files.
+ */
+@RunWith(Parameterized.class)
+public class AsRepoInitTest {
+
+    private final ParserTest.TestCase tc;
+
+    @Parameters(name="{0}")
+    public static Collection<Object[]> data() throws IOException {
+        final List<Object []> result = new ArrayList<>();
+        for(int i=0; i < 100; i++) {
+            final ParserTest.TestCase tc = ParserTest.TestCase.build(i);
+            if(tc != null) {
+                result.add(new Object[] { tc });
+            }
+        }
+        return result;
+
+    }
+
+    public AsRepoInitTest(ParserTest.TestCase tc) {
+        this.tc = tc;
+    }
+
+    @Test
+    public void checkResultAsRepoInit() throws Exception {
+        StringBuilder repoInit = new StringBuilder();
+        final List<Operation> expectedResult;
+        try {
+            expectedResult = new RepoInitParserService().parse(tc.input);
+            for (Operation o : expectedResult) {
+                repoInit.append(o.asRepoInitString());
+            }
+        } finally {
+            tc.close();
+        }
+
+        List<Operation> ops = new RepoInitParserService().parse(new StringReader(repoInit.toString()));
+        assertEquals(expectedResult.size(), ops.size());
+        // FIXME: assert operations are equivalent

Review comment:
       to compare the reconstructed repoinit script S with the original O, I see two options:
   * 1) implement `equals` in all `Operation` classes and use that to compare the result of parsing S and O
   * 2) normalize the test input String (removing extra spaces etc) and compare to the normalized `repoInit` string
   
   If needed for 2) we might slightly change the test inputs for things that the normalization wouldn't catch, to make testing easier




-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] bdelacretaz commented on pull request #9: SLING-10236 : Introduce Operation.asRepoInitString

Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#issuecomment-810133173


   I have refactored `AsRepoInitTest` and `ParserTest` to share more code and better express that in both cases we want to get the same output, as verified by the `-output` test scripts.
   
   Please cross-check, from my point of view I think this would be good to merge. 


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] anchela commented on pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
anchela commented on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#issuecomment-807999041


   @bdelacretaz , in my original draft in the cp-fm-converter i compared original, expected repo-init with the generated out and  verified that SetAclPrincipalBased works. what exactly do you expect not working for the SetAclPrincipalBased?
   I already noted in the JIRA ticket that toString is not implemented in a consistent way. but unfortunately the getParameterDescription method is not public (and also not consistently implemented).... so {{toString]} was the best present today..... what would you suggest?


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] bdelacretaz merged pull request #9: SLING-10236 : Introduce Operation.asRepoInitString

Posted by GitBox <gi...@apache.org>.
bdelacretaz merged pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9


   


-- 
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.

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

[GitHub] [sling-org-apache-sling-repoinit-parser] sonarcloud[bot] commented on pull request #9: SLING-10236 : Introduce Operation.asRepoInitString (initial draft with FIXMEs and TODOs)

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #9:
URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/9#issuecomment-805973760


   SonarCloud Quality Gate failed.
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=BUG)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=VULNERABILITY)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=SECURITY_HOTSPOT)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&resolved=false&types=CODE_SMELL)
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/90.png' alt='93.1%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&metric=new_coverage&view=list) [93.1% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&metric=new_coverage&view=list)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/5.png' alt='3.9%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&metric=new_duplicated_lines_density&view=list) [3.9% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-repoinit-parser&pullRequest=9&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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.

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