refactor, test: Improve undo/redo history and add critical edge-case tests#83
Conversation
c85dad8 to
9fd9596
Compare
|
Hey there! Thanks for your hard work. The feature looks solid, but I've found several critical issues that need fixing before we can merge. There's significant code duplication in history_manager.py. The handlers for group_add, group_remove, and group_delete appear twice in both _execute_undo_operation and _execute_redo_operation methods. Remove the duplicate elif blocks and keep only the first occurrence of each. The edit command in cli.py (around line 69) doesn't record history with history.push(), which breaks undo/redo for edits. You need to add a history_op that captures both the original and new alias states, then push it to the history stack. Your test assertions in test_undo_redo.py (lines 96 and 105) don't match the actual messages from history_manager.py. Update them to match the exact format: " The history_manager.py calls storage.add() and storage.remove() with a record_history=False parameter, but storage.py doesn't have this parameter. You'll need to add it to both methods with a default value of True, and implement the history recording logic within them. There's also a potential index calculation bug in the selective undo/redo by ID methods around lines 367-398. Please verify the logic is correct for ID 1 being the most recent operation. On the positive side, your test coverage and the documentation is excellent. The approach is sound, we just need to address these technical issues and split the PR. Please create smaller PRs in the future. Thanks for understanding! |
9fd9596 to
3f6dcfa
Compare
|
@Valentin-v-Todorov Request you to check now. |
|
The main blocker is that history_manager.py calls storage.add() and storage.remove() with record_history=False, but these methods in storage.py (lines 93 and 104) don't accept this parameter. You need to add record_history: bool = True to both method signatures and wrap the existing self.history.push() calls in if record_history: blocks. Second issue is in the edit command (cli.py lines 120-154). It's calling storage.remove() and storage.add() with record_history=True, which creates two separate history entries instead of one edit entry. Change both calls to record_history=False and let the manual history_op push at line 132-140 handle it properly. Also, please double-check that there are no duplicate elif blocks for group operations in history_manager.py - I see group_add, group_remove, and group_delete handlers that might be duplicated in the refactoring. Once these are fixed, this will be ready to merge. The functionality and tests are solid, just need these technical corrections. 🚀 |
This is already addressed i think. |
3f6dcfa to
47a63b4
Compare
|
@Valentin-v-Todorov Please check now. |
|
@skalwaghe-56 All previous issues have been resolved, tests are properly aligned with the new messages, and the code quality is excellent. GGs |
PR Checklist
What does this PR do?
Also, this is the 3rd and last installment PR in implementing the undo/redo functionality.
🔧 Configuration Fix: Pip Packaging Issue
Problem: pip install -e . was failing with "Multiple top-level packages discovered in a flat-layout: ['alix', 'backups']".
Root Cause: Setuptools was auto-discovering both alix (Python package) and backups (data directory) as Python packages.
Solution: Added explicit package discovery configuration to exclude non-package directories.
Related Issue
Fixes #25.
Type of change
Testing
By running pytest and testing the features actually by yourself.
Additional Information
I am extremely sorry for the huge PR by the time I realized I couldn't revert and split it :/ Thanks for your understanding!