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
| 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
There was a problem hiding this comment.
is cloud_recorder.CloudRecorder able to handle initial_values being set to None?
edit: I have checked, yes it does
| 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
|
applied some of the requested changes |
| 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.
is cloud_recorder.CloudRecorder able to handle initial_values being set to None?
edit: I have checked, yes it does
2 participants
Solves issue #656
Changes
Add 1 to every packet number requested, change packet request detection
Tests