ExchangeHandler stats fix #1205
Conversation
|
Any chance I can twist your arm into writing up 1-2 basic smoke tests for this? Should be structurally similar to how the |
|
Sure, fair enough. Almost any bug fix that wasn't caught by CI should add a new test. By structurally similar, I assume you mean that the test should send a request and then validate that the stats are updated? |
also, a bugfix for getting stats before any requests are made
Yes |
|
|
||
| for attr, exchange_cls in self._exchanges.items(): | ||
| if hasattr(self, attr): | ||
| raise AttributeError( | ||
| "Unable to set manager on attribute `{0}` which is already " | ||
| "present on the class: {1}".format(attr, getattr(self, attr)) | ||
| ) | ||
| manager: ExchangeManager[Any, Any, Any] = ExchangeManager(self._peer, peer.cancel_token) | ||
| manager: ExchangeManager[Any, Any, Any] | ||
| manager = ExchangeManager(self._peer, exchange_cls.response_cmd_type, peer.cancel_token) |
There was a problem hiding this comment.
IIUC the classproperty is being used to reduce the length of this lookup from exchance_cls.request_class.reponse_cmd_type to exchange_cls.response_cmd_type.
I'm not convinced that the classproperty decorator is worth this given that it required us to silence mypy.
There was a problem hiding this comment.
Got rid of the mypy silencer, so I kept the class property thing.
What was wrong?
Trying to call
get_stats()on the exchange handler was crashing. Broken during #1184How was it fixed?
Keep a link to all the exchange managers, and use them to call get stats.
Cute Animal Picture