Allow sync operations to modify the sync filter #284
Conversation
…dification of context during sync operations
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
Tests are almost exclusively additions, so we have good guarantees of consistent behaviour here. The tests that have been updated have mostly been to use real objects instead of mocks, which makes the outcome of the tests feel more robust!
No blockers, just comments and questions!
| :param params: DEPRECATED: USE Filter.from_template() INSTEAD | ||
| :type params: dict|str | ||
| """ | ||
| if params is not None: |
There was a problem hiding this comment.
Worth adding a deprecation warning here, or not worth the effort just to warn us? :)
| """ | ||
| if other is None: | ||
| return self | ||
| # create a list of partition filters, deduplicating them between the two filter objects |
There was a problem hiding this comment.
Almost feels like partitions are more semantically sets rather than lists?
There was a problem hiding this comment.
Yeah, I guess that would make this simpler to just use a set.
There was a problem hiding this comment.
Hmm, I forgot sets don't maintain order. While the Filter class does not explicitly try to preserve that, it does make sense to do so. To avoid adding a dep for OrderedSet, I will leave this as is for now.
| result = middleware(prepared_context) | ||
|
|
||
| # return the prepared context back | ||
| context.join(prepared_context) |
There was a problem hiding this comment.
Ah, I think this is what I was looking for!
| # During the initializing stage, we want to make sure to synchronize the transfer session | ||
| # object between the composite and children contexts, using whatever child context's | ||
| # transfer session object that was updated on the context during initialization | ||
| current_stage = stage or self._stage |
There was a problem hiding this comment.
So the logic that was here is removed in favour of join, mostly? But with some tweaks? And then join is called...
There was a problem hiding this comment.
Yeah since join() is called in nearly the same spot that triggered this code, it made sense to just move this logic to join() and add the new functionality there.
| context = TestSessionContext() | ||
|
|
||
| sync_filter = mock.Mock(spec=Filter) | ||
| sync_filter = Filter("a") |
There was a problem hiding this comment.
Always feels like a confidence boosting change to switch out a Mock for a real object in a test.
2 participants
Summary
Filterto:.join()interface in parity with.prepare()CompositeSessionContext.update()logic into.join()Release notes
Filterconstructor usage utilizing parameters-- useFilter.from_templateinsteadTODO
FilterReviewer guidance
INTEGRATION_TEST=1 pytest -x kolibri/core/auth/test/test_morango_integration.pyIssues addressed
Closes #282