-
Notifications
You must be signed in to change notification settings - Fork 763
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Impact being bound to checks is limiting when composing new rules #693
Comments
Can you clarify what you mean with:
|
If the rule’s violation impact depends on which check actually failed, then the checks’ impacts can be bound (or overridden) when the rule is configured. Concretely, the For instance:
|
Had a quick talk with the team. I think your proposal overcomplicates things. It would certainly take a bunch of reworking of internal tools, so I'm not a fan of that. I think we should simplify this instead. we think it would be better to simply allow an |
Yes, I think it's a good intermediary workaround. In fact my first answer draft included something similar to what you describe, as an alternative to my comment above :-) but since the clarification was specifically about check/impact association, I ended up pruning that part… By the way, FWIW this issue is currently not critical to us (DAISY). I just logged it here after @marcysutton asked for API feedback for 3.0 the other day on Twitter. |
It makes little sense to me to have the impact on checks, since the same check can be used by different rules in different ways. I would prefer it was only a property on the It might be more complicated - some rule failures matter a lot in practice, and some don't. I'm not sure there is a simple way of describing how that works for automation... |
I agree that it makes more sense for the impact to be attached to the check, however, it makes sense to attach that impact on the rule to the check. We could make this backwards compatible by making the list of checks a list of check names or objects that contain a check name and an impact. E.g. Currently, the checks section for the label rule looks like this:
It might look like this if we wished to override/specify the impact of the check within the context of the rule:
|
I think that just overcomplicates things. Very few rules can actually come up with issues of different levels of impact ( |
Having a different impact for checks is only one use case. The other common case is having two different rules with different impacts for the same check. Custom rule writers quite often require this and right now we have to duplicate the check to achieve this. |
@dylanb That would also be solved if you can simply override the impact at the rule level. What is the benefit of overriding the impact of the check at the rule level? |
What you are proposing is that the ability to have different impact level failures for a single rule is not valuable. I am not convinced that your assertion is true. If it is true, then what we should be talking about is a fundamental rearchitecting of this feature. If we don't do that, then adding an override at the rule level is only a partial solution. |
Why I'm proposing we make this configurable at the rule level, is because I think that changing the impact of a rule should be something users can do without having to learn how impact aggregation works in checks. You shouldn't have to be an advanced user to do something that's this trivial. Now we could add your check configuration idea in addition to that. The change I'm suggesting doesn't prevent us from adding your solution as well. But I suspect that adding this "basic" feature will be enough that people won't have to duplicate checks anymore, or at least only in very rare cases. |
Verified by configuring the rule |
@padmavemulapati you are correct. Put in a pr that should fix this. |
@padmavemulapati should be good to test this again. |
Thank you @straker , its working now. I am able to see the modified impact at rule level update, instead checks level. |
included as a new feature in internal release notes 7/29/2020 |
The impact property ("minor", "moderate", "serious", or "critical") is currently bound to a check. It means that when composing a new rule, this rule will necessarily inherit the impacts of the underlying checks.
In my understanding however, a check is an "abstract" atomic piece of logic. A rule on the contrary is the "concrete" implementation of a given accessible guideline. The actual impact of a violation only makes sense for a given rule.
For instance, the
non-empty-title
check has aserious
impact. But there may be cases where a rule checks the presence of empty titles but isn’t about a serious violation (we have such rules in the digital publishing context). In that case, we have no other choice than implementing a custom check with a lesser impact, even if the implementation logic is exactly the same asnon-empty-title
.A possible improvement IMO would be to associate the impact to the check only when the rule is composed, but not bake it in the check.
The text was updated successfully, but these errors were encountered: