Improvements to IPConnection #26
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26 +/- ##
==========================================
+ Coverage 43.96% 45.03% +1.06%
==========================================
Files 18 18
Lines 655 675 +20
==========================================
+ Hits 288 304 +16
- Misses 367 371 +4 ☔ View full report in Codecov by Sentry. |
OCopping
force-pushed
the
improve_ip_connection
branch
from
189347d
to
b7f6601
Compare
|
If you two are happy with the decorator approach to try and clean up the functions inside Connection objects, I might try and move them into a separate script to then be imported. I would have to make them more generalised though (e.g. instead of looking at I have tested similar decorators in |
| async with self.lock: | ||
| await func(*args, **kwargs) | ||
|
|
||
| return cast(_AsyncFuncType, with_lock) |
There was a problem hiding this comment.
I think I would prefer to have to do
with self.lock:
...
than this honestly. It is more conventional and it allows minimising the time the lock is held without having to split it out into a function.
There was a problem hiding this comment.
The old implementation wrapped all the contents of the functions in the context manager though. I can see why this would be nice for if only part of the function was in the CM, but I feel like the decorator approach is cleaner if that isn't the case.
|
|
||
| @property | ||
| def lock(self) -> asyncio.Lock: | ||
| return self._lock |
There was a problem hiding this comment.
This should be removed if _with_lock is removed. I am wary that this suggests that it can be generally used outside the class, and it should not.
There was a problem hiding this comment.
Originally I didn't have this property, I only created it as the linter complained. The intention is that this would only be used in the decorator so wrap the function in the context manager, not use outside the class.
| else: | ||
| await func(*args, **kwargs) | ||
|
|
||
| return cast(_AsyncFuncType, check_connected) |
There was a problem hiding this comment.
In this case I would prefer to make a wrapper around StreamWriter and StreamReader (StreamConnection?) where the connection is either open, or an exception is raised. This would simplify the typing and avoid having to do isinstance and is not None everywhere. So, IPConnection would make a StreamConnection, the connection is guaranteed to be open as soon as the object is instantiated and then it can call send and recv without having to check every time if it is valid.
I feel like this shouldn't be necessary. I expect am misunderstanding something here, but the asyncio API for this seems clunky.
Thoughts?
There was a problem hiding this comment.
I did realise this probably does the same thing as SerialConnection does in #24, except that uses the socket module. I feel like that would be the better way to approach this.
It is worth noting that currently is written to be synchronous.
|
I think this is superseded by the refactor for pyright in #61 |
No description provided.