Conversation
8f7a7dc to
3ab740a
Compare
DustieDog
left a comment
There was a problem hiding this comment.
Vest wasn't reviewed, commented other files as needed.
| extends RefCounted | ||
| class_name _DiffHistoryEncoder | ||
|
|
||
| var sanitize: bool = true |
There was a problem hiding this comment.
What're your thoughts on renaming to sanitized or is_sanitized? Same for passthrough-history-encoder and redundant-history-encoder
There was a problem hiding this comment.
I wouldn't use passive language, since sanitize is not something being done to the class, but something done by the class. Then again, I don't see a case where we wouldn't want to sanitize, let me get rid of that toggle.
There was a problem hiding this comment.
Passthrough feels unclear to me here. What's your thoughts on FullHistoryEncoder to contrast DiffHisteryEncoder? Open to other suggestions as well.
| return result | ||
|
|
||
| # Returns earliest new tick as int, or null | ||
| func apply(tick: int, snapshots: Array[_PropertySnapshot], sender: int = 0): |
There was a problem hiding this comment.
Having _RedundantHistoryEncoder.apply() have a different return signature to _PassthroughHistoryEncoder.apply() and _DiffHistoryEncoder.apply() feels inconsistent. Ideas?
There was a problem hiding this comment.
Same with the rest of their signatures - I've intentionally didn't bring them under a shared base class. An option is to have a separate get_earliest_updated() method or similar, though I wouldn't force consistency here.
There was a problem hiding this comment.
Cool, where it's internal I'm fine leaving them mismatched for now.
|
|
||
| # Returns earliest new tick as int, or null | ||
| func apply(tick: int, snapshots: Array[_PropertySnapshot], sender: int = 0): | ||
| var earliest_new_tick = null |
There was a problem hiding this comment.
Thoughts on using -1 as the default return value?
Co-authored-by: Dustie <77035922+DustieDog@users.noreply.github.com>
…efactor/history-encoder
This reverts commit ee2a985.
Refs #358