feat: refactor typing and add better diff#17
Open
FriendlyUser1 wants to merge 6 commits into
Open
Conversation
Reviewer's GuideRefactors typing across the database, comparison, and UI layers, introduces a structured EntryDetails type, and enhances the Streamlit UI to distinguish exclusive, common, and conflicting KeePass items with field‑level conflict highlighting. Sequence diagram for viewing conflicting KeePass entries with field-level highlightingsequenceDiagram
actor User
participant StreamlitApp as app.toggle_entry
participant DatabaseUtils as get_entry_details
participant Comparison as compare_entries
participant EntryModal as show_entry_details
User->>StreamlitApp: Click Conflicting Items tab
loop For each entry in differences["common_entries"]
StreamlitApp->>DatabaseUtils: get_entry_details(kp1, entry)
StreamlitApp->>DatabaseUtils: get_entry_details(kp2, entry)
StreamlitApp->>Comparison: compare_entries(entry1_details, entry2_details)
Comparison-->>StreamlitApp: conflicts (List[str])
alt conflicts not empty
User->>StreamlitApp: Click view buttons for DB1/DB2
StreamlitApp->>EntryModal: show_entry_details(entry_details, key, conflicts)
EntryModal-->>User: Render details with conflicting fields highlighted
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In the 'Common Entries' tab logic,
compare_entries(entry1_details, entry2_details) == 1will never be true becausecompare_entriesreturns a list of differing keys; you probably want to check for an empty/non-empty list (e.g.if compare_entries(...):orif len(...) > 0:) to distinguish conflicting from identical entries. - In the 'Conflicting Entries' tab,
conflictsis overwritten on each loop iteration and then only the last entry's conflicts are used to decide whether to show 'No conflicting entries found'; consider tracking a separate boolean or accumulating conflicts so the final message reflects all entries, not just the last one. - For
compare_entries, consider adding an explicit return type (e.g.List[str]) and usingis Noneinstead of== Noneto match the rest of the typing-focused refactor and avoid subtle type/identity pitfalls.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the 'Common Entries' tab logic, `compare_entries(entry1_details, entry2_details) == 1` will never be true because `compare_entries` returns a list of differing keys; you probably want to check for an empty/non-empty list (e.g. `if compare_entries(...):` or `if len(...) > 0:`) to distinguish conflicting from identical entries.
- In the 'Conflicting Entries' tab, `conflicts` is overwritten on each loop iteration and then only the last entry's conflicts are used to decide whether to show 'No conflicting entries found'; consider tracking a separate boolean or accumulating conflicts so the final message reflects all entries, not just the last one.
- For `compare_entries`, consider adding an explicit return type (e.g. `List[str]`) and using `is None` instead of `== None` to match the rest of the typing-focused refactor and avoid subtle type/identity pitfalls.
## Individual Comments
### Comment 1
<location path="KeePassDiff/utils/comparison.py" line_range="6-11" />
<code_context>
+from KeePassDiff.utils.database import EntryDetails
def compare_databases(
db1_data: Dict[str, Set[str]], db2_data: Dict[str, Set[str]]
-) -> Dict:
</code_context>
<issue_to_address>
**issue (bug_risk):** "groups_only_in_db2" uses db2 - db2, which will always be empty.
`groups_only_in_db2` is calculated as `db2_data["groups"] - db2_data["groups"]`, which is always an empty set and makes this part of the comparison ineffective. This likely needs to be `db2_data["groups"] - db1_data["groups"]` to match the logic used elsewhere.
</issue_to_address>
### Comment 2
<location path="KeePassDiff/app.py" line_range="148-155" />
<code_context>
-
with tabs[1]:
- st.subheader("Conflicting Entries")
+ st.subheader("Common Entries")
if differences["common_entries"]:
+ conflicts = False
for entry in differences["common_entries"]:
+
+ entry1_details = get_entry_details(kp1, entry)
+ entry2_details = get_entry_details(kp2, entry)
+ if compare_entries(entry1_details, entry2_details) == 1:
+ continue
+ conflicts = True
</code_context>
<issue_to_address>
**issue (bug_risk):** compare_entries is treated as returning an int, but it actually returns a list.
Here `compare_entries(entry1_details, entry2_details) == 1` is always `False` because `compare_entries` returns a list of conflicting field names (or `[]`), not an int. As a result, `continue` never runs and all common entries are shown. If you want to skip entries with conflicts and only show identical ones, this should instead check for an empty list (e.g. `if compare_entries(...) != []:` or `if len(compare_entries(...)) > 0:`), depending on the exact semantics you want.
</issue_to_address>
### Comment 3
<location path="KeePassDiff/app.py" line_range="144-153" />
<code_context>
st.write("None")
-
with tabs[1]:
- st.subheader("Conflicting Entries")
+ st.subheader("Common Entries")
if differences["common_entries"]:
</code_context>
<issue_to_address>
**issue (bug_risk):** The `conflicts` variable is reused per-entry but also used as a global flag, which can misreport whether any conflicts exist.
Within the conflicting entries tab, `conflicts` is overwritten on each loop iteration with `compare_entries`’ result, then `if len(conflicts) == 0:` is used afterward to decide whether to show "No conflicting entries found". This only reflects the last entry processed—earlier conflicts are ignored if the final entry has none. Track a separate flag (e.g. `any_conflicts = any_conflicts or bool(conflicts)`) to accurately determine whether any conflicts were found overall.
</issue_to_address>
### Comment 4
<location path="tests/test_keepassdiff.py" line_range="100-101" />
<code_context>
def __init__(self):
self.path = [None, "group"]
+ @property
+ def is_root_group(self):
+ return False
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case that covers the `is_root_group` behaviour for root vs non-root groups in `get_entries_set`.
Since the double now exposes `is_root_group`, please add a test KeePass fixture containing both a root group (`is_root_group == True`) and a non-root subgroup. Assert that the root group path is excluded from `groups` and the non-root subgroup is included, so changes to the root-group detection logic are caught by tests.
Suggested implementation:
```python
def __init__(self):
# default non-root group used in most tests
self.path = [None, "group"]
@property
def is_root_group(self):
return False
class DummyRootGroup(DummyGroup):
def __init__(self):
# path corresponding to the KeePass root group
self.path = [None]
@property
def is_root_group(self):
return True
class DummyKP:
entries = [DummyEntry()]
# include both a root group and a non-root subgroup to exercise
# root-group filtering logic in get_entries_set
groups = [DummyRootGroup(), DummyGroup()]
expected_db2_entries = db2_data["entries"]
assert expected_db1_entries == {"a", "b"}, f"db1 entries: {expected_db1_entries}"
assert expected_db2_entries == {"b", "c"}, f"db2 entries: {expected_db2_entries}"
def test_get_entries_set_filters_root_group_from_groups():
"""
Ensure that get_entries_set excludes the root group from the groups set,
while still including non-root subgroups.
"""
kp = DummyKP()
db_data = get_entries_set(kp)
groups = db_data["groups"]
# root group (with only [None] as its path) should not appear in groups
assert "" not in groups
# non-root subgroup from DummyGroup (path [None, "group"]) should be present
assert "group" in groups
```
1. Ensure `get_entries_set` is imported into `tests/test_keepassdiff.py` (for example: `from keepassdiff import get_entries_set`) if it is not already.
2. If `get_entries_set` represents groups differently (e.g. including full paths like `"group/subgroup"` or using tuples), adjust the assertions in `test_get_entries_set_filters_root_group_from_groups` accordingly:
- Replace `assert "" not in groups` with a check that the representation of the root group is absent.
- Replace `assert "group" in groups` with the correct expected representation of the non-root subgroup.
</issue_to_address>
### Comment 5
<location path="tests/test_keepassdiff.py" line_range="163" />
<code_context>
@@ -148,15 +152,15 @@ def test_diff_and_merge_on_programmatic_db():
</code_context>
<issue_to_address>
**suggestion (testing):** Extend database comparison tests to validate all fields in the `differences` dict, not just the entries-related ones.
Since `compare_databases` now returns a full `Dict[str, List[str]]`, including group-related keys, add assertions for `groups_only_in_db1`, `groups_only_in_db2`, and `common_groups` as well. In `test_diff_and_merge_on_programmatic_db`, explicitly check the expected values (or emptiness) of these keys so changes to group comparison logic are detected by the tests.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These changes include the typing enhancements from #16 plus new common / conflicting ui with key highlighting for better user experience
Resolves #13 and addresses #9
Summary by Sourcery
Improve KeePass database diff UI with separate common/conflicting item views and richer typed entry metadata.
New Features:
Bug Fixes:
Enhancements: