You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by "kwin (via GitHub)" <gi...@apache.org> on 2023/10/05 13:42:55 UTC

[PR] SLING-12076 Export JcrResourceChange with additional method [sling-org-apache-sling-jcr-resource]

kwin opened a new pull request, #39:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/39

   getUserData()


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

Re: [PR] SLING-12076 Export JcrResourceChange with additional method [sling-org-apache-sling-jcr-resource]

Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on PR #39:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/39#issuecomment-1748931451

   Alternatively we could move the getter with a more JCR agnostic name ("context"?) to `org.apache.sling.api.resource.observation.ResourceChange` and leave `JcrResourceChange` not exported.


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

Re: [PR] SLING-12076 Export JcrResourceChange with additional method [sling-org-apache-sling-jcr-resource]

Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin merged PR #39:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/39


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

Re: [PR] SLING-12076 Export JcrResourceChange with additional method [sling-org-apache-sling-jcr-resource]

Posted by "henrykuijpers (via GitHub)" <gi...@apache.org>.
henrykuijpers commented on code in PR #39:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/39#discussion_r1403618842


##########
src/main/java/org/apache/sling/jcr/resource/api/JcrResourceChange.java:
##########
@@ -16,27 +16,37 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.sling.jcr.resource.internal;
+package org.apache.sling.jcr.resource.api;
 
 import org.apache.sling.api.resource.observation.ResourceChange;
 
 /**
- * Extension of {@code ResourceChange} to support user id (if available)
+ * Extension of {@code ResourceChange} to support user id and user data (if available)
  */
-public class JcrResourceChange extends ResourceChange {
+public final class JcrResourceChange extends ResourceChange {

Review Comment:
   Marking this final is not very nice for consumers / implementers 



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

Re: [PR] SLING-12076 Export JcrResourceChange with additional method [sling-org-apache-sling-jcr-resource]

Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #39:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/39#discussion_r1404094590


##########
src/main/java/org/apache/sling/jcr/resource/api/JcrResourceChange.java:
##########
@@ -16,27 +16,37 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.sling.jcr.resource.internal;
+package org.apache.sling.jcr.resource.api;
 
 import org.apache.sling.api.resource.observation.ResourceChange;
 
 /**
- * Extension of {@code ResourceChange} to support user id (if available)
+ * Extension of {@code ResourceChange} to support user id and user data (if available)
  */
-public class JcrResourceChange extends ResourceChange {
+public final class JcrResourceChange extends ResourceChange {

Review Comment:
   Well it could, but it doesn't in this case, therefore I don't see a reason to open this here right now (due to restricting evolution of this class unnecessarily). Once again, if we come up with a good use case we can still change that, but let us always start with the most restricted API. @henrykuijpers If you have a convincing use case for extending please tell us.



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

Re: [PR] SLING-12076 Export JcrResourceChange with additional method [sling-org-apache-sling-jcr-resource]

Posted by "henrykuijpers (via GitHub)" <gi...@apache.org>.
henrykuijpers commented on code in PR #39:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/39#discussion_r1404146108


##########
src/main/java/org/apache/sling/jcr/resource/api/JcrResourceChange.java:
##########
@@ -16,27 +16,37 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.sling.jcr.resource.internal;
+package org.apache.sling.jcr.resource.api;
 
 import org.apache.sling.api.resource.observation.ResourceChange;
 
 /**
- * Extension of {@code ResourceChange} to support user id (if available)
+ * Extension of {@code ResourceChange} to support user id and user data (if available)
  */
-public class JcrResourceChange extends ResourceChange {
+public final class JcrResourceChange extends ResourceChange {

Review Comment:
   No, I'm fine with leaving it restricted. The class, at this moment, doesn't contain a lot of logic. 
   
   There have been quite some cases where it was very useful that "internal" implementations were freely accessible (for example JcrLabeledResource in Adobe's codebase, it contains quite some code to read the right properties etc) - creating a wrapper would still be possible



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

Re: [PR] SLING-12076 Export JcrResourceChange with additional method [sling-org-apache-sling-jcr-resource]

Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #39:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/39#discussion_r1403619774


##########
src/main/java/org/apache/sling/jcr/resource/api/JcrResourceChange.java:
##########
@@ -16,27 +16,37 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.sling.jcr.resource.internal;
+package org.apache.sling.jcr.resource.api;
 
 import org.apache.sling.api.resource.observation.ResourceChange;
 
 /**
- * Extension of {@code ResourceChange} to support user id (if available)
+ * Extension of {@code ResourceChange} to support user id and user data (if available)
  */
-public class JcrResourceChange extends ResourceChange {
+public final class JcrResourceChange extends ResourceChange {

Review Comment:
   This is deliberate, no consumer should ever extend this class. Why would you want to? What is your usecase? That would unnecessarily restrict evolution of this class (to expose other attributes in the future)...



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

Re: [PR] SLING-12076 Export JcrResourceChange with additional method [sling-org-apache-sling-jcr-resource]

Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on PR #39:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/39#issuecomment-1824741078

   @cziegeler Any concerns with this, otherwise I will  merge beginning of next week.


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

Re: [PR] SLING-12076 Export JcrResourceChange with additional method [sling-org-apache-sling-jcr-resource]

Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #39:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/39#discussion_r1403620085


##########
src/main/java/org/apache/sling/jcr/resource/api/JcrResourceChange.java:
##########
@@ -16,27 +16,37 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.sling.jcr.resource.internal;
+package org.apache.sling.jcr.resource.api;
 
 import org.apache.sling.api.resource.observation.ResourceChange;
 
 /**
- * Extension of {@code ResourceChange} to support user id (if available)
+ * Extension of {@code ResourceChange} to support user id and user data (if available)
  */
-public class JcrResourceChange extends ResourceChange {
+public final class JcrResourceChange extends ResourceChange {

Review Comment:
   Remember it was a non-exported class before!



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

Re: [PR] SLING-12076 Export JcrResourceChange with additional method [sling-org-apache-sling-jcr-resource]

Posted by "henrykuijpers (via GitHub)" <gi...@apache.org>.
henrykuijpers commented on code in PR #39:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/39#discussion_r1403735663


##########
src/main/java/org/apache/sling/jcr/resource/api/JcrResourceChange.java:
##########
@@ -16,27 +16,37 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-package org.apache.sling.jcr.resource.internal;
+package org.apache.sling.jcr.resource.api;
 
 import org.apache.sling.api.resource.observation.ResourceChange;
 
 /**
- * Extension of {@code ResourceChange} to support user id (if available)
+ * Extension of {@code ResourceChange} to support user id and user data (if available)
  */
-public class JcrResourceChange extends ResourceChange {
+public final class JcrResourceChange extends ResourceChange {

Review Comment:
   It could contain nice "ootb" behavior and as an implementer it could be nice to reuse it and/or to extend it. We've done this a lot with various classes and it's always unfortunate when they are in internal packages or when they are final. (Devil's advocate speaking 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

Re: [PR] SLING-12076 Export JcrResourceChange with additional method [sling-org-apache-sling-jcr-resource]

Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #39:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/39#issuecomment-1759646125

   Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=CODE_SMELL)
   
   [![84.6%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60-16px.png '84.6%')](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&metric=new_coverage&view=list) [84.6% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&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

Re: [PR] SLING-12076 Export JcrResourceChange with additional method [sling-org-apache-sling-jcr-resource]

Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #39:
URL: https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/39#issuecomment-1760137050

   Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&resolved=false&types=CODE_SMELL)
   
   [![84.6%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60-16px.png '84.6%')](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&metric=new_coverage&view=list) [84.6% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-jcr-resource&pullRequest=39&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