-
Notifications
You must be signed in to change notification settings - Fork 0
β‘ Bolt: optimize JsonFilePracticeRepository.save_items with in-place updates #79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -46,15 +46,24 @@ def save_items(self, items: list[LearningItem]) -> None: | |||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| storage = self._load_storage() | ||||||||||||||||||||||||
| raw_items = storage.get("items", []) | ||||||||||||||||||||||||
| existing_items = [] | ||||||||||||||||||||||||
| if isinstance(raw_items, list): | ||||||||||||||||||||||||
| existing_items = [ | ||||||||||||||||||||||||
| _item_from_dict(entry) for entry in raw_items if isinstance(entry, dict) | ||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||
| by_id = {existing.id: existing for existing in existing_items} | ||||||||||||||||||||||||
| for item in items: | ||||||||||||||||||||||||
| by_id[item.id] = item | ||||||||||||||||||||||||
| storage["items"] = [_item_to_dict(entry) for entry in by_id.values()] | ||||||||||||||||||||||||
| if not isinstance(raw_items, list): | ||||||||||||||||||||||||
| raw_items = [] | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| # β‘ Bolt: Optimize batch saves by updating raw dicts in-place | ||||||||||||||||||||||||
| # instead of full deserialize/serialize loop (avoids O(N) penalty) | ||||||||||||||||||||||||
| new_items_by_id = {str(item.id): _item_to_dict(item) for item in items} | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| for entry in raw_items: | ||||||||||||||||||||||||
| if isinstance(entry, dict): | ||||||||||||||||||||||||
| entry_id = str(entry.get("id")) | ||||||||||||||||||||||||
| if entry_id in new_items_by_id: | ||||||||||||||||||||||||
| entry.clear() | ||||||||||||||||||||||||
| entry.update(new_items_by_id.pop(entry_id)) | ||||||||||||||||||||||||
|
Comment on lines
+58
to
+61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current logic
Suggested change
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| for new_item in new_items_by_id.values(): | ||||||||||||||||||||||||
| raw_items.append(new_item) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| storage["items"] = raw_items | ||||||||||||||||||||||||
| self._save_storage(storage) | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| def list_attempts(self) -> list[Attempt]: | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
storage["items"]contains duplicate records with the sameid, this loop updates only the first one and then removes the pending update viapop, leaving later duplicates stale. That is a behavior regression from the previous implementation, which rebuilt a by-id map and collapsed duplicates on each save; with the new logic,list_items()can return conflicting versions of the same item and downstream code that materializes{item.id: item}may end up using an outdated record.Useful? React with πΒ / π.