Semver2 fix 656#681
Conversation
faretek1
left a comment
faretek1
left a comment
There was a problem hiding this comment.
I left some general comments about the code but there is nothing critically obvious to the logic to me. As long as you have tested it well, it is probably safe to commit
|
|
||
| @property | ||
| def var_sets_since_first(self) -> int: | ||
| return self.var_stets_since_first |
There was a problem hiding this comment.
Yes, but we cannot fix the typo itself (due to semver)
| self.last_var_set = time.time() | ||
|
|
||
| def get_var(self, var, *, recorder_initial_values={}): | ||
| def get_var(self, var, *, recorder_initial_values: Optional[dict] = None): |
There was a problem hiding this comment.
If you really want to replicate the original behaviour you need to check if recorder... is None: recorder... = {}
There was a problem hiding this comment.
The original behaviour was potentially dangerous due to a dangerous default
| return self.recorder.get_var(var) | ||
|
|
||
| def get_all_vars(self, *, recorder_initial_values={}): | ||
| def get_all_vars(self, *, recorder_initial_values: Optional[dict] = None): |
There was a problem hiding this comment.
Same as above for none check
There was a problem hiding this comment.
Another option is to just make it a required field instead of optional
|
|
||
|
|
||
| class ScratchCloud(BaseCloud): | ||
| class ScratchCloud(LogCloud): |
There was a problem hiding this comment.
Does this mean all scratch clouds now print logs? Is this desired?
There was a problem hiding this comment.
A log cloud is just a cloud that has a logs method
|
|
||
| def __init__(self, request_name, *, on_call, cloud_requests, thread=True, enabled=True, response_priority=0, debug=False): | ||
| def __init__( | ||
| self, |
There was a problem hiding this comment.
Somewhat unrelated but a note that typing could be added here
| def __init__( | ||
| self, | ||
| cloud, | ||
| used_cloud_vars=["1", "2", "3", "4", "5", "6", "7", "8", "9"], |
There was a problem hiding this comment.
Old code was like this too but list literals shouldnt be used as default arguments
| if len(request_id) == 7 and request_id[-1] == "9": | ||
| # A lost packet was re-requested | ||
| self._request_packet_from_memory(request_id[1:], int(raw_request)) | ||
| self._request_packet_from_memory(request_id[:-1], int(raw_request) + 1) |
There was a problem hiding this comment.
I presume this is the actual fix? Have you tested it? I presume so? The main thing to test is if it can handle large payloads. Does that work?
There was a problem hiding this comment.
I tested it with the code and project from the issue that this fixes (with large payloads)
| logs = getattr(self.cloud, "logs", None) | ||
| if logs is None: | ||
| return | ||
| extradata = logs(limit=35)[ | ||
| ::-1 | ||
| ] # Reverse result so oldest activity is first |
There was a problem hiding this comment.
Is this also part of the core fix? In which cases would there not be a logs method? Is getattr really the best solution here?
There was a problem hiding this comment.
the logs method exists for LogClouds (which a tw connection is not) and the original behaviour would be raising an AttributeError which would instantly get caught so this is actually equivalent
There was a problem hiding this comment.
i only added the getattr thingy to please ty/mypy
|
applied some of the requested changes |
2 participants
Solves issue #656
Changes
Add 1 to every packet number requested, change packet request detection
Tests