Refactor syncobj_admin #129
Conversation
Extract parts the `Utility` class responsible for network communications into the `TcpUtility`, so other parties could easily reuse it.
pysyncobj/utility.py
Outdated
| self.__result = None | ||
| self.__error = None | ||
|
|
||
| def sendMessage(self, node, message): |
There was a problem hiding this comment.
I suggest to rename it into 'executeCommand' and make it return result / error. Otherwise it's not obvious why sendMessage run polling loop inside and wait for disconnect.
pysyncobj/syncobj_admin.py
Outdated
| return False | ||
|
|
||
|
|
||
| def syncobjAdmin(args): |
pysyncobj/utility.py
Outdated
| if time.time() > deadline: | ||
| self.__connection.disconnect() | ||
|
|
||
| return self.getResult() or self.getError() |
There was a problem hiding this comment.
May be at least return tuple / dict? How can we distinguish errors from non-errors? (this is optional, if you don't think it's a good idea or don't have time - I'll merge as is)
There was a problem hiding this comment.
Well, yeah, this is the issue.
That's why in the original code I was explicitly checking if the result is None and returning the error.
Unfortunately, I don't have a better interface for that, except maybe raising an exception.
There was a problem hiding this comment.
What about return (self.getResult(), self.getError()) ?
There was a problem hiding this comment.
I think it would be more appropriate to raise an exception.
After all, this is what exceptions are useful for :)
| result = util.executeCommand(data.connection, message) | ||
|
|
||
| if isinstance(result, str): | ||
| return result |
There was a problem hiding this comment.
May be use only dict everywhere, and put "error" inside dict with error text for error handling?
There was a problem hiding this comment.
Well, the result could be anything (whatever the server returned).
I just "replicated" the original code.
There was a problem hiding this comment.
Ok, can merge it but looks not so cool. May be a separate issue for refactor?
Extract parts of the
Utilityclass responsible for network communications into theTcpUtility, so other parties could easily reuse it.