🧊 🧑🏼💻 Rules: Fallback rule implementation (aka rules that will run only if no other rule was executed)#592
🧊 🧑🏼💻 Rules: Fallback rule implementation (aka rules that will run only if no other rule was executed)#592
Conversation
Issue 562
ArthurD1
dismissed
their stale review
Having no rules configured might trigger an error
|
I’ll have a look. |
hartym
changed the title
hartym
changed the title
|
I've just added a follow-up commit that fixes this issue by adding a simple guard clause upon entering the function. The rules matching will now be skipped entirely when no rules are configured. I've tried the patch with from the repo. |
There was a problem hiding this comment.
We're on a good path, but not quite there yet.
I pushed some fixes related to comments I made, especially the fact that the changes to match item was now skipping matches from different subtrees (because it now used return instead of yield from.
I also added some tests for the potential issue we discussed at pycon, and I have a good and a bad news: the bug discussed does not and did not exist: the harp_apps.rules.models.rulesets.BaseRuleSet.match method "flattens" the rules, level by level, and all subtrees are considered. The bad news is, the new default behaviour is having hard time there, because it means that if we have two subtrees with default rules, having a nondefault rule match in the first subtree will make the second subtree default not matching, which is not what I believe we want.
For example:
a:
foo: ...
b:
__default__: ...If "the "foo" rule match, then default will never be considered.
I added a failing test for this, and I guess we need to address that before we can merge. Another thing needed before it can be merged is some bits of documentation: one should be able to find references to default in the docs, even if it's only a few sentences + examples here and there (no need to overdo it). An entry should also be added to the docs/changelogs/unreleased.rst file.
| def _match_level(rules, against): | ||
| default = None | ||
| has_match = False | ||
| for pattern, rules in rules: | ||
| if pattern == DEFAULT_RULE_MATCHER: | ||
| default = rules | ||
| continue | ||
|
|
||
| if pattern.match(against): | ||
| if hasattr(rules, "items"): | ||
| yield from rules.items() | ||
| else: | ||
| yield from rules | ||
| has_match = True | ||
| return Itemize(rules) | ||
|
|
||
| if not has_match and default: | ||
| return Itemize(default) |
There was a problem hiding this comment.
As far as I understand, this changes the behaviour of rules, allowing only one match to be made amongst one level (by returning instead of yielding from). The «itemize» object brings more confusion than clarity, on top of that.
the current test suite is a bit incomplete on the rules side, but catches an issue there (see harp_apps.rules.tests.test_models_rulesets.test_multilevel).
| if not self.rules: | ||
| return |
There was a problem hiding this comment.
Is that related to the current patch or a preexisting bug ? Asking because I want to make sure I understand what have caused the change in behaviour if it worked previously.
Anyway, can't do real harm to check, but probably the empty rules should be iterable anyway (empty iterator). More a note for myself here than anything else.
There was a problem hiding this comment.
This guards against the TypeError: 'NoneType' object is not iterable when no rule is configured. This was reported above.
hartym
changed the title
|
Thank you for the detailed feedback! This is really appreciated. The feature indeed seems more involved than I first thought. I'm still interested in working on this but I must let you know that I'm really busy this month. I apologize if this causes any inconvenience. |
No worries, take your time, your help is also really appreciated. |
hartym
changed the title
hartym
changed the title
hartym
changed the title
3 participants
Issue #562