You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by "stefanseifert (via GitHub)" <gi...@apache.org> on 2023/05/10 08:45:29 UTC
[GitHub] [sling-org-apache-sling-testing-jcr-mock] stefanseifert opened a new pull request, #20: SLING-11868 jcr-mock: MockAuthorizable must not use java.nio.file.Paths
stefanseifert opened a new pull request, #20:
URL: https://github.com/apache/sling-org-apache-sling-testing-jcr-mock/pull/20
https://issues.apache.org/jira/browse/SLING-11868
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-jcr-mock] enapps-enorman commented on a diff in pull request #20: SLING-11868 jcr-mock: MockAuthorizable must not use java.nio.file.Paths
Posted by "enapps-enorman (via GitHub)" <gi...@apache.org>.
enapps-enorman commented on code in PR #20:
URL: https://github.com/apache/sling-org-apache-sling-testing-jcr-mock/pull/20#discussion_r1190229377
##########
src/main/java/org/apache/sling/testing/mock/jcr/MockAuthorizable.java:
##########
@@ -67,7 +66,7 @@ abstract class MockAuthorizable implements Authorizable {
intermediatePath = "/home/users"; // NOSONAR
}
}
- this.path = Paths.get(intermediatePath, this.id).toString();
+ this.path = intermediatePath + "/" + this.id;
Review Comment:
Sorry about that.
How about using something like org.apache.jackrabbit.oak.commons.PathUtils.concat(intermediatePath, this.id) to improve the readability and provide some protection against illegal arguments?
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-jcr-mock] enapps-enorman commented on a diff in pull request #20: SLING-11868 jcr-mock: MockAuthorizable must not use java.nio.file.Paths
Posted by "enapps-enorman (via GitHub)" <gi...@apache.org>.
enapps-enorman commented on code in PR #20:
URL: https://github.com/apache/sling-org-apache-sling-testing-jcr-mock/pull/20#discussion_r1190287705
##########
src/main/java/org/apache/sling/testing/mock/jcr/MockAuthorizable.java:
##########
@@ -67,7 +66,7 @@ abstract class MockAuthorizable implements Authorizable {
intermediatePath = "/home/users"; // NOSONAR
}
}
- this.path = Paths.get(intermediatePath, this.id).toString();
+ this.path = intermediatePath + "/" + this.id;
Review Comment:
Wouldn't it be safe to add the extra oak-commons dependency as long as it is using the same (minimum) version as the other oak-* dependencies that already exist in the pom?
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-jcr-mock] sonarcloud[bot] commented on pull request #20: SLING-11868 jcr-mock: MockAuthorizable must not use java.nio.file.Paths
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-testing-jcr-mock/pull/20#issuecomment-1542643918
SonarCloud Quality Gate failed. [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&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.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-jcr-mock] stefanseifert commented on a diff in pull request #20: SLING-11868 jcr-mock: MockAuthorizable must not use java.nio.file.Paths
Posted by "stefanseifert (via GitHub)" <gi...@apache.org>.
stefanseifert commented on code in PR #20:
URL: https://github.com/apache/sling-org-apache-sling-testing-jcr-mock/pull/20#discussion_r1190744250
##########
src/main/java/org/apache/sling/testing/mock/jcr/MockAuthorizable.java:
##########
@@ -67,7 +66,7 @@ abstract class MockAuthorizable implements Authorizable {
intermediatePath = "/home/users"; // NOSONAR
}
}
- this.path = Paths.get(intermediatePath, this.id).toString();
+ this.path = intermediatePath + "/" + this.id;
Review Comment:
i've created https://issues.apache.org/jira/browse/SLING-11870 to streamline the oak dependency in jcr-mock, we can approach this after #21 was merged.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-jcr-mock] stefanseifert closed pull request #20: SLING-11868 jcr-mock: MockAuthorizable must not use java.nio.file.Paths
Posted by "stefanseifert (via GitHub)" <gi...@apache.org>.
stefanseifert closed pull request #20: SLING-11868 jcr-mock: MockAuthorizable must not use java.nio.file.Paths
URL: https://github.com/apache/sling-org-apache-sling-testing-jcr-mock/pull/20
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-jcr-mock] kwin commented on pull request #20: SLING-11868 jcr-mock: MockAuthorizable must not use java.nio.file.Paths
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-testing-jcr-mock/pull/20#issuecomment-1542086263
@stefanseifert We haven’t reached consensus there and you didn’t approve the PR either
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-jcr-mock] enapps-enorman commented on a diff in pull request #20: SLING-11868 jcr-mock: MockAuthorizable must not use java.nio.file.Paths
Posted by "enapps-enorman (via GitHub)" <gi...@apache.org>.
enapps-enorman commented on code in PR #20:
URL: https://github.com/apache/sling-org-apache-sling-testing-jcr-mock/pull/20#discussion_r1190586268
##########
src/main/java/org/apache/sling/testing/mock/jcr/MockAuthorizable.java:
##########
@@ -67,7 +66,7 @@ abstract class MockAuthorizable implements Authorizable {
intermediatePath = "/home/users"; // NOSONAR
}
}
- this.path = Paths.get(intermediatePath, this.id).toString();
+ this.path = intermediatePath + "/" + this.id;
Review Comment:
FYI: I've just proposed https://github.com/apache/sling-org-apache-sling-testing-jcr-mock/pull/21 whose implementation would remove this specific code path. The users/group would be backed with a real node in the repository rather than the fake path that was used before. If that PR is approved, then this one should be no longer needed.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-jcr-mock] stefanseifert commented on a diff in pull request #20: SLING-11868 jcr-mock: MockAuthorizable must not use java.nio.file.Paths
Posted by "stefanseifert (via GitHub)" <gi...@apache.org>.
stefanseifert commented on code in PR #20:
URL: https://github.com/apache/sling-org-apache-sling-testing-jcr-mock/pull/20#discussion_r1190251142
##########
src/main/java/org/apache/sling/testing/mock/jcr/MockAuthorizable.java:
##########
@@ -67,7 +66,7 @@ abstract class MockAuthorizable implements Authorizable {
intermediatePath = "/home/users"; // NOSONAR
}
}
- this.path = Paths.get(intermediatePath, this.id).toString();
+ this.path = intermediatePath + "/" + this.id;
Review Comment:
yes, this would be better. the class you mention is not available in the classpath.
i assume it's part of `org.apache.jackrabbit:oak-commons`. we could add it, but then we would need to manage the version of this dependency in all contexts as well fitting to the actual oak version used.
so it would be better to find a better methods in the dependencies we already have in place to not over-complicate things here.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-jcr-mock] stefanseifert commented on pull request #20: SLING-11868 jcr-mock: MockAuthorizable must not use java.nio.file.Paths
Posted by "stefanseifert (via GitHub)" <gi...@apache.org>.
stefanseifert commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-testing-jcr-mock/pull/20#issuecomment-1543439324
superseded by #21 which eliminates the problematic code line using java.nio.file.Paths
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-jcr-mock] stefanseifert commented on pull request #20: SLING-11868 jcr-mock: MockAuthorizable must not use java.nio.file.Paths
Posted by "stefanseifert (via GitHub)" <gi...@apache.org>.
stefanseifert commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-testing-jcr-mock/pull/20#issuecomment-1541630575
i was under the impression we will get windows build by default in the future for all sling CI runs [SLING-11843](https://issues.apache.org/jira/browse/SLING-11843) (looking forward for this) - it's fine for me to wait to get this in place
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-jcr-mock] sonarcloud[bot] commented on pull request #20: SLING-11868 jcr-mock: MockAuthorizable must not use java.nio.file.Paths
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-testing-jcr-mock/pull/20#issuecomment-1541602896
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-jcr-mock&pullRequest=20&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.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-jcr-mock] kwin commented on pull request #20: SLING-11868 jcr-mock: MockAuthorizable must not use java.nio.file.Paths
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on PR #20:
URL: https://github.com/apache/sling-org-apache-sling-testing-jcr-mock/pull/20#issuecomment-1541611710
@stefanseifert Should we enable Windows CI builds on this module (https://cwiki.apache.org/confluence/display/SLING/Sling+module+descriptor#Slingmoduledescriptor-BuildingwithmultipleJDKversionsandoperatingsystems)?
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org