feat: #192 add severity levels that are not violations #409
Conversation
There was a problem hiding this comment.
This is a good addition, and it's good to relax the interpretation of conforms. However, there are other places that now need to be changed too. A quick look revealed the section "Conformance Checking", which formally defines the term "conforms". This needs to take the additional flexibility into account. Other places that rely on "conforms" such as sh:node then don't need to be adjusted further.
…language improvement
|
@HolgerKnublauch I also rechecked the spec and found only the "Conformance Checking" section that needed an update. The change is included in the latest commit. |
|
@ajnelson-nist I've added the test cases for |
| <p> | ||
| Validation engines MAY treat <code>sh:Info</code> and <code>sh:Warning</code> as non-violating based on | ||
| options passed to the engine. By default, they are treated as violations. | ||
| </p> |
There was a problem hiding this comment.
I think my one last hangup on this PR is this escape and the lack of demonstration of a "test configuration record".
The tests @bergos added, 4 and 5, are fine demonstrations of a validation result set with conformance of true and the non-violation levels. (Thank you, Thomas.)
Can the test suite support a new test 6, mostly duplicative of 4 or 5, but instead using sh:Warning and a conformance of true? How would test suite conformance be reproducible?
I feel like another property in the ValidationReport would be helpful, that logs whether its sh:conformance value was set to "excuse" sh:Info or sh:Warning severity results from tripping sh:conformance into false. [] a sh:ValidationReport ; sh:excuses sh:Warning ; ...? sh:permits, sh:permitsSeverity, or sh:excusesSeverity might work too.
There was a problem hiding this comment.
@ajnelson-nist I understand, and I also would like to add tests for it, but there are two problems I tried to avoid:
- The feature is optional, and as an implementor, I would like to pick only those tests that match the features supported by the implementation to see if any tests fail. That means the tests must be tagged to test a specific optional feature.
- The feature is controlled by a parameter given to the engine. A data structure for the parameters would be required that can be added to the test to configure the engine.
I've created an issue #431 to discuss that topic, as these two problems are not specific to this PR. If it's OK, I would merge this PR now and use the issue to track the missing tests?
There was a problem hiding this comment.
This feels like a pretty significant design decision:
A. Make a general runtime-configuration data structure.
B. Represent a specific runtime-configuration value having an impact on a top-level sh:ValidationReport feature.
To do B, do we need to do A?
This particular case feels like it's toeing a line I hadn't appreciated before dividing "optional behavior" from "optional feature".
More practically speaking, looking at the surround text, I think this section needs a bit more tweaking. We currently have this scale:
| Severity | Violation? |
|---|---|
| Trace | No |
| Debug | No |
| Info | Non-critically yes |
| Warning | Non-critically yes |
| Violation | Yes |
Should the "Validation engines MAY treat ..." paragraph describe "conformance," rather than whether each sh:ValidationResult Boolean-is-or-is-not a violation?
There was a problem hiding this comment.
We'll need to address this, but after lengthy discussion on today's call, I believe it unlikely we'll forget to. Please do leave the conversation "unresolved" in the GitHub interface so we can still see this later.
This PR closes #192 "Add severity levels that are not violations". It contains the following changes:
sh:Traceandsh:Debugseverity levels that aren't violations.sh:Infoandsh:Warningas non-violating.sh:conformsof being true when there are no results to being true if there are no results with a severity level that is a violation.A rule based on RDF classes for sh:conforms would have been preferred, but since it's possible to dynamically set sh:Info and sh:Warning being a violation, that is not possible.
Allowing validation engines to treat levels as non-violating but keeping the default makes this a non-breaking change.