Refactor the FIELD pseudo-cause into the catch-all events and a field filter #69
Conversation
🤖 zincr found 0 problems , 1 warningDetails on how to resolve are provided below Large CommitsChecks all commits for large additions to a single file. Large commits should be reviewed more carefully for potential copyright and licensing issues This file contains a substantial change, please review to determine if the change comes from an external source and if there are any copyright or licensing issues to be aware of
|
🤖 zincr found 1 problem , 1 warningDetails on how to resolve are provided below ApprovalsAll proposed changes must be reviewed by project maintainers before they can be merged Not enough people have approved this pull request - please ensure that 1 additional user, who have not contributed to this pull request approve the changes.
Large CommitsChecks all commits for large additions to a single file. Large commits should be reviewed more carefully for potential copyright and licensing issues This file contains a substantial change, please review to determine if the change comes from an external source and if there are any copyright or licensing issues to be aware of
|
nolar
changed the title
nolar
force-pushed
the
no-field-pseudo-cause
branch
from
59aa73d to
be7e701
Compare
|
|
||
|
|
||
| @pytest.fixture() | ||
| @pytest.fixture(params=[ | ||
| pytest.param([('op', ('some-field',), 'old', 'new')], id='with-field-diff'), |
There was a problem hiding this comment.
Operation. Usually it is one of: add, change, remove. In this case, it is irrelevant, as the object-under-test is the cause & handlers, not the diff — but a list of 4-item tuples is expected in the internal for-cycle, no matter what's inside, so we give it.
| def test_catchall_handlers_with_field_found(cause_with_diff, registry, register_fn): | ||
| register_fn(some_fn, event=None, field='some-field') | ||
| handlers = registry.get_handlers(cause_with_diff) | ||
| assert handlers |
There was a problem hiding this comment.
Do you assert when this is not empty? Does it mean it catches all the handlers?
There was a problem hiding this comment.
Yes. It means, that something is there, i.e. non-empty.
There is only one handler registered in the previous line, so technically it also means that all handlers are returned (for that field: since both then handler and the cause contain this field — in this specific test).
The
FIELDpseudo-cause was introduced as an easy way to mark the per-field handlers (@kopf.on.field), and then to select them on the event handling. The field handlers were handled separately, after all the object handlers.Over time, this pseudo-cause was merged into the overall handling routine, and has become a problem: it prevents further refactoring (of event/cause detection and handling), which in turn is needed for the test coverage.
This PR is a set of changes extracted from a bigger branch for the ease of code review (one small part at a time). It does NOT change the behaviour of the framework or of any handlers; it just changes the internal implementation of it:
FIELDpseudo-causes in the code.event==None) withfieldproperty set are now used — i.e. any event as long as this field is in the diff. Efficiently, it means only theUPDATEevents — same as before.In addition, the tests of handler selection were improved to reflect all cases; and to make the test ids more readable.