[WIP] Timeout queue for posting updates#2
Conversation
| url: str | ||
|
|
||
|
|
||
| def eq(self, other): |
There was a problem hiding this comment.
TypedDicts can only contain type annotations. This can't be here, and I don't think it needs to be anyway.
| """ | ||
|
|
||
| # For the added and removed pages, we simply add them to the new set if there is any | ||
| self.added += other_changed_set.added |
There was a problem hiding this comment.
I feel like this is not necessary since you don't have timeout to deal with for page additions/removals and therefore don't pass them forward to the next changeset
There was a problem hiding this comment.
We do, because what we intend to do is always forward those into the master changeset, since our current logic is
get_new_changes -> merge_to_master -> check_timeout_at_master -> send_message_from_master
|
|
||
|
|
||
| # We compare if there are any changes in the same page and coalece | ||
| # Horribly underoptimized, we will get back to this later |
There was a problem hiding this comment.
I agree. If you want to do this you should probably use a different data structure e.g. a dict that maps pages (or page IDs) to their changes
There was a problem hiding this comment.
suggestion: refactor Page into a dataclass so you can implement __hash__ and then change ChangeSet.changed into a Dict[Page, Tuple[List[PropertyChange], datetime]] or something
| properties: Dict[str, Property] | ||
| url: str | ||
|
|
||
| @override |
There was a problem hiding this comment.
Is @override necessary here? Does Page have a __hash__ method in the first place?
|
|
||
| @override | ||
| def __hash__(self) -> int: | ||
| return hash(page_id) |
There was a problem hiding this comment.
| return hash(page_id) | |
| return hash(self.page_id) |
| """Pages that were removed in the diff""" | ||
| changed: List[Tuple[Page, List[PropertyChange]]] | ||
| changed: Dict[Page, Tuple[List[PropertyChange], datetime]] | ||
| """Pages that had their properties changed in the diff. The key is the page id and the value is the list of |
There was a problem hiding this comment.
Update this docstring accordingly.
| # For each change we check if its already in our dict | ||
| for page, (propertyList, timestamp) in other_changed_set.changed: | ||
| if page in self.changed: | ||
| new_timestamp = self.changed[page][1] |
There was a problem hiding this comment.
| new_timestamp = self.changed[page][1] | |
| property_changes, new_timestamp = self.changed[page] |
and then use property_changes on line 59
| self.removed += other_changed_set.removed | ||
|
|
||
| # For each change we check if its already in our dict | ||
| for page, (propertyList, timestamp) in other_changed_set.changed: |
There was a problem hiding this comment.
Are you iterating through the dict here? If so,
| for page, (propertyList, timestamp) in other_changed_set.changed: | |
| for page, (propertyList, timestamp) in other_changed_set.changed.items(): |
| def is_empty(self) -> bool: | ||
| return len(self.added) == 0 and len(self.removed) == 0 and len(self.changed) == 0 | ||
|
|
||
| def merge_set(self, other_changed_set): |
There was a problem hiding this comment.
Add type annotations
There was a problem hiding this comment.
How? It wont let me do typehint to ChangeSet
|
Seems to be working now, but will need unit tested before we can merge. (https://discord.com/channels/1250426749058289715/1258870098790191234/1261162378268250244) |
2 participants
No description provided.