[GitHub] [jena] afs commented on a change in pull request #870: Add public getters to SHACL constraint implementations

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

afs commented on a change in pull request #870:
URL: https://github.com/apache/jena/pull/870#discussion_r533625107



##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/PatternConstraint.java
##########
@@ -53,6 +53,18 @@ public PatternConstraint(String pattern, String flagsStr) {
         this.pattern = Pattern.compile(pattern, flags);
     }
 
+    public Pattern getPattern() {

Review comment:
       Minor: I'd prefer not the Pattern because it is an internal term, and then have the pattern string as `getPattern()`.
   
   It is exposing the information once, not in two different ways.

##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/CardinalityConstraint.java
##########
@@ -38,7 +38,15 @@ public void validateNodeShape(ValidationContext vCxt, Graph data, Shape shape, N
         // Node shape with cardinality. Can this usefully be checked for in the parser?
         throw new ShaclParseException("Cardinality constraint on a node shape");
     }
-    
+
+    public int getMinCount(){

Review comment:
       Not sure if this the best thing to do. The subclass for max and min already have accessors.
   
   Being an abstract class, CardinalityConstraint can't be created directly.
   
   "MaxCount" have a "getMinCount" function is a bit strange. If there were a general min and max at the same time constraint, then on the superclass would make more sense but there isn't.
   
   Please remove theses and use overrides in the subclasses.
   

##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/QualifiedValueShape.java
##########
@@ -50,6 +50,22 @@ public QualifiedValueShape(Shape sub, int qMin, int qMax, boolean qDisjoint) {
         this.qDisjoint = qDisjoint;
     }
 
+    public Shape getSub() {
+        return sub;
+    }
+
+    public int getqMin() {

Review comment:
       AutoCase does not work out! And "Q" does work that well either! How about `public int qMin()`?

##########
File path: jena-shacl/src/main/java/org/apache/jena/shacl/engine/constraint/JViolationConstraint.java
##########
@@ -35,6 +35,10 @@ public JViolationConstraint(boolean generateViolation) {
         this.generateViolation = generateViolation;
     }
 
+    public boolean isGenerateViolation() {

Review comment:
       Minor: "generateViolation" read better.




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



---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org