fix(core): get_children with include_data=True uses GetChildren2 types #515
Conversation
kazoo/protocol/connection.py
Outdated
| @@ -380,7 +381,8 @@ def _read_response(self, header, buffer, offset): | |||
| # Determine if watchers should be registered | |||
| watcher = getattr(request, 'watcher', None) | |||
| if not client._stopped.is_set() and watcher: | |||
| if isinstance(request, GetChildren): | |||
| if (isinstance(request, GetChildren) | |||
| or isinstance(request, GetChildren2)): | |||
There was a problem hiding this comment.
visually indented line with same indent as next logical line
There was a problem hiding this comment.
@saffroy FYI isinstance() can take multiple classes as a tuple, so that'd be the easiest way to clean this up:
if isinstance(request, (GetChildren, GetChildren2)):
There was a problem hiding this comment.
Ah, that should do. And I learned something!
kazoo/protocol/connection.py
Outdated
| @@ -380,7 +381,8 @@ def _read_response(self, header, buffer, offset): | |||
| # Determine if watchers should be registered | |||
| watcher = getattr(request, 'watcher', None) | |||
| if not client._stopped.is_set() and watcher: | |||
| if isinstance(request, GetChildren): | |||
| if (isinstance(request, GetChildren) | |||
There was a problem hiding this comment.
@saffroy do you know if GetChildren is even used? Or is it always GetChildren2?
There was a problem hiding this comment.
AIUI both are used: GetChildren2 when (at the get_children API level) we request data in the response, GetChildren otherwise.
So, what's unclear to me is how to best please the linter here without degrading the code. Maybe separate into two if statements that branch to identical statements? Doesn't look quite right to me, but I don't care that much.
There was a problem hiding this comment.
Ah, that makes sense re GetChildren2 vs GetChildren.
There's a straightforward way to fix this linting issue, I overlooked it previously, so I'll leave a comment above.
|
I am not sure why GitHub is now marking the branches as conflicting and requiring an update before merging... regardless I went ahead and updated and will merge via rebase which should drop the merge commit from the history. Just waiting on the tests to finish. |
|
@saffroy can you address the hound violation above? I personally don't see it as a blocker in this particular instance, but I don't have the permissions here to bypass it... Please rebase to get rid of my merge commit and then I'll go ahead and merge this... |
|
nudge @saffroy |
Closes #514