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/01/26 17:38:08 UTC

[GitHub] [sling-org-apache-sling-dynamic-include] santiagozky opened a new pull request #19: Extend the IncludeGenerator interface with the Sling Request 

santiagozky opened a new pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19


   Extend the IncludeGenerator interface with the Sling Request to give more flexibility to potential implementations of the interface.


----------------------------------------------------------------
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-dynamic-include] santiagozky edited a comment on pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
santiagozky edited a comment on pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19#issuecomment-767708146


   The current interface is a bit limited. I want to create a custom IncludeGenerator that relies in some 'context' that could be obtained from the original request.
   
   If changing the interface is not an option, maybe I can add a second method?


----------------------------------------------------------------
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-dynamic-include] sonarcloud[bot] commented on pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19#issuecomment-817823231


   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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&resolved=false&types=CODE_SMELL)
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&metric=new_coverage&view=list)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include] santiagozky commented on a change in pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
santiagozky commented on a change in pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19#discussion_r601516107



##########
File path: src/main/java/org/apache/sling/dynamicinclude/api/IncludeGenerator.java
##########
@@ -17,13 +17,15 @@
  * under the License.
  */
 
-package org.apache.sling.dynamicinclude.generator;
+package org.apache.sling.dynamicinclude.api;
+
+import org.apache.sling.api.SlingHttpServletRequest;
 
 /**
  * Include generator interface
  */
 public interface IncludeGenerator {
     String getType();
 
-    String getInclude(String url);
+    String getInclude(SlingHttpServletRequest request,String url);

Review comment:
       the issue here is that the url is not just taken from the request as it is. It is previously checked to do things like removing the query part, map it,encode, etc (in https://github.com/apache/sling-org-apache-sling-dynamic-include/blob/master/src/main/java/org/apache/sling/dynamicinclude/IncludeTagFilter.java#L149),.
   we could remove the url parameter, but that means any IncludeGenerator would be responsible for doing that




-- 
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-dynamic-include] sonarcloud[bot] commented on pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19#issuecomment-767709363


   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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&resolved=false&types=CODE_SMELL)
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&metric=new_coverage&view=list)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include] rombert commented on pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
rombert commented on pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19#issuecomment-768130982


   Thanks for the PR @santiagozky . As this is a non-trivial addition, can you please file a Jira issue first under https://issues.apache.org/jira/browse/SLING so we can discuss the best way of implementing 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-dynamic-include] santiagozky closed pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
santiagozky closed pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19


   


----------------------------------------------------------------
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-dynamic-include] rombert commented on a change in pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
rombert commented on a change in pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19#discussion_r605591995



##########
File path: src/main/java/org/apache/sling/dynamicinclude/Configuration.java
##########
@@ -66,11 +65,7 @@
       @AttributeDefinition(name="Resource types", description="Filter will replace components with selected resource types", cardinality = Integer.MAX_VALUE)
       String include$_$filter_config_resource$_$types() default "";
       
-      @AttributeDefinition(name = "Include type", description = "Type of generated include tags", options = {
-          @Option(label = "Apache SSI", value = "SSI"),
-          @Option(label = "ESI", value = "ESI"),
-          @Option(label = "Javascript", value = "JSI")
-      })
+      @AttributeDefinition(name = "Include type", description = "Type of generated include tags")

Review comment:
       Makes sense. It might help if we listed the built-in implementations.




-- 
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-dynamic-include] sonarcloud[bot] commented on pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19#issuecomment-792904228


   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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&resolved=false&types=CODE_SMELL)
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&metric=new_coverage&view=list)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include] rombert merged pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
rombert merged pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19


   


-- 
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-dynamic-include] rombert commented on a change in pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
rombert commented on a change in pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19#discussion_r600332843



##########
File path: src/main/java/org/apache/sling/dynamicinclude/Configuration.java
##########
@@ -66,11 +65,7 @@
       @AttributeDefinition(name="Resource types", description="Filter will replace components with selected resource types", cardinality = Integer.MAX_VALUE)
       String include$_$filter_config_resource$_$types() default "";
       
-      @AttributeDefinition(name = "Include type", description = "Type of generated include tags", options = {
-          @Option(label = "Apache SSI", value = "SSI"),
-          @Option(label = "ESI", value = "ESI"),
-          @Option(label = "Javascript", value = "JSI")
-      })
+      @AttributeDefinition(name = "Include type", description = "Type of generated include tags")

Review comment:
       Any reason for removing this?

##########
File path: src/main/java/org/apache/sling/dynamicinclude/api/IncludeGenerator.java
##########
@@ -17,13 +17,15 @@
  * under the License.
  */
 
-package org.apache.sling.dynamicinclude.generator;
+package org.apache.sling.dynamicinclude.api;
+
+import org.apache.sling.api.SlingHttpServletRequest;
 
 /**
  * Include generator interface
  */
 public interface IncludeGenerator {
     String getType();
 
-    String getInclude(String url);
+    String getInclude(SlingHttpServletRequest request,String url);

Review comment:
       I think it's confusing to pass in the request and a string named `url` I see that the `url` parameter is based on the sling request, so maybe we can have a different approach 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.

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

[GitHub] [sling-org-apache-sling-dynamic-include] santiagozky commented on pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
santiagozky commented on pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19#issuecomment-767708146


   The current interface is a bit limited. I want to create a custom IncludeGenerator that relies in some 'context' that could be obtained from the original request.


----------------------------------------------------------------
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-dynamic-include] santiagozky commented on pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
santiagozky commented on pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19#issuecomment-768166578


   hi @rombert. Sure. I've created it https://issues.apache.org/jira/browse/SLING-10096


----------------------------------------------------------------
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-dynamic-include] santiagozky commented on a change in pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
santiagozky commented on a change in pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19#discussion_r601510841



##########
File path: src/main/java/org/apache/sling/dynamicinclude/Configuration.java
##########
@@ -66,11 +65,7 @@
       @AttributeDefinition(name="Resource types", description="Filter will replace components with selected resource types", cardinality = Integer.MAX_VALUE)
       String include$_$filter_config_resource$_$types() default "";
       
-      @AttributeDefinition(name = "Include type", description = "Type of generated include tags", options = {
-          @Option(label = "Apache SSI", value = "SSI"),
-          @Option(label = "ESI", value = "ESI"),
-          @Option(label = "Javascript", value = "JSI")
-      })
+      @AttributeDefinition(name = "Include type", description = "Type of generated include tags")

Review comment:
       well, if the point is to allow extra implementations, that means we cannot longer have a fixed list of options. otherwise you cannot choose whatever you implement




-- 
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-dynamic-include] sonarcloud[bot] commented on pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19#issuecomment-792916447


   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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&resolved=false&types=CODE_SMELL)
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&metric=new_coverage&view=list)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include] santiagozky commented on pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
santiagozky commented on pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19#issuecomment-818701825


   thank you @rombert 


-- 
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-dynamic-include] santiagozky commented on pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
santiagozky commented on pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19#issuecomment-817822320


   Sorry for the delay @rombert. I was too busy and this feltout of priority for me.  I've added the built-in generators inthe configuration label and added the method javadoc


-- 
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-dynamic-include] rombert commented on a change in pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
rombert commented on a change in pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19#discussion_r605601669



##########
File path: src/main/java/org/apache/sling/dynamicinclude/api/IncludeGenerator.java
##########
@@ -17,13 +17,15 @@
  * under the License.
  */
 
-package org.apache.sling.dynamicinclude.generator;
+package org.apache.sling.dynamicinclude.api;
+
+import org.apache.sling.api.SlingHttpServletRequest;
 
 /**
  * Include generator interface
  */
 public interface IncludeGenerator {
     String getType();
 
-    String getInclude(String url);
+    String getInclude(SlingHttpServletRequest request,String url);

Review comment:
       I think that would be ok, granted that we:
   
   - find a better name for it ( normalizedUrl?)
   - add better documentation to the interface and the method, explaining why we pass in an apparently redundant parameter




-- 
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-dynamic-include] santiagozky commented on pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
santiagozky commented on pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19#issuecomment-797470082


   @rombert 
   sorry it took long. I had to leave this for a bit.
   It complains about one line without unit testing. do you need it?


----------------------------------------------------------------
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-dynamic-include] sonarcloud[bot] removed a comment on pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19#issuecomment-792904228


   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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&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-dynamic-include&pullRequest=19&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&resolved=false&types=CODE_SMELL)
   
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&metric=new_coverage&view=list)  
   [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-dynamic-include&pullRequest=19&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-dynamic-include] santiagozky commented on pull request #19: Extend the IncludeGenerator interface with the Sling Request

Posted by GitBox <gi...@apache.org>.
santiagozky commented on pull request #19:
URL: https://github.com/apache/sling-org-apache-sling-dynamic-include/pull/19#issuecomment-792823558


   sorry I never updated this. fell out of priority for me and havent have time to get around it.


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