JENA-2009: Implement Visitor Pattern for SHACL Constraints#882
JENA-2009: Implement Visitor Pattern for SHACL Constraints#882afs merged 5 commits intoapache:masterfrom
Conversation
|
Please could you put the vitro in a consistent place in the constraints? Just before printCompact would be fine as many of them are there already. |
|
I am unsure that default methods on the visitor are the way to go. An abstract class "ConstraintBase" has been the style up until Java8 and now can have a mixin/trait to do the same. Reason: robustness. When writing complex visitors it is good to have a way for the programmer to know they have covered all cases and be notified when a new constraint is added. Otherwise it is possible to miss a case and compile time and and it becomes a runtime problem. |
afs
left a comment
afs
left a comment
There was a problem hiding this comment.
Overall, looks good. Some refinement for the long term in comments.
| import org.apache.jena.shacl.engine.constraint.*; | ||
|
|
||
| public interface ConstraintVisitor { | ||
| default void visit(ReportConstraint constraint){} |
There was a problem hiding this comment.
This one can be skipped. It's a dummy that only exists to carry a message. It can't be an executable constraint.
There was a problem hiding this comment.
We must implement ReportConstraint.visit(ConstraintVisitor), though. Empty implementation for minimal friction?
There was a problem hiding this comment.
That'll do fine. The validation operations are throw new UnsupportedOperationException() but no-op works just as well for the visitor. (I suspect that ReportConstraint shouldn't exist at all! But from memory, passing the information it carries means adding other parameters in many places which even if purer, is verbose.)
| default void visit(StrLanguageIn constraint){} | ||
| default void visit(StrMinLengthConstraint constraint){} | ||
| default void visit(JViolationConstraint constraint){} | ||
| default void visit(DatatypeConstraint constraint){} |
There was a problem hiding this comment.
Move to be with ClassConstaint. (The ideal order is as in section 4: https://www.w3.org/TR/shacl/#core-components).
The J* then go separately.
We'll need a "ConstraitExt" for extensions (not a strong point of the visitor pattern!) but that can wait for now.
Also remove empty default implementations in ConstraintVisitor
... to reflect order in in W3C SHACL 1.0 Recommendation
4 participants
Notes:
visitmethod toConstraintinterface. I experimented with providing a default implementation (callingvisitor.visit(Constraint)) to avoid having to add an implementation to all implementing classes. This approach does not work, which I did not expect. Learned something about Java method invocation target selection there.visitmethod non-abstract subclasses ofConstraint. The pattern suggests they should be namedaccept, but jena usesvisitwherever I checked. I guess it's better to stick with the existing flavor.visitor.visit(this). Please let me know if you feel differently. Using the PR in my client code, so I'm positive it works.