Repository navigation
Feat/add sync - #9
Conversation
feat: add uid property, similar method to OrgAgendaItem feat: add from_path, get_item, update, update_item to OrgAgendaFile fix: correct day OrgRegex to work with latin locales refactor: create _list method to be able to reuse it in sync docs: how to run the tests style: rename the argument parser args to match the subcommand style: fix typos style: run black on some files
refactor: sync conditionals so they are clearer refactor: remove unused update and update_item from OrgAgendaFile docs: update the sync usage text
fix: set the default logger level to INFO fix: avoid mutable arguments in checker It was polluting the state between tests fix: prevent duplicate timestamps when start == end style: remove dead code style: run black on some files
refactor: parametrize dry_run tests
fix: add more debugging information when two items are flagged as different
BartSte
left a comment
BartSte
left a comment
There was a problem hiding this comment.
Many thanks for your contribution. The sync feature was something I also wanted so I am very happy with this!
Codex picked up the following. Can you check if these are actually problems?
• The new sync path has functional issues: dry-run still mutates the filesystem for missing paths, and item comparison can miss removed properties. These can cause user-visible incorrect sync behavior.
Full review comments:
-
[P2] Respect dry-run before creating sync files — /home/barts/code/personal/python/khalorg/src/khalorg/commands.py:303-309
When khalorg sync --dry-run is run with a new org_file or state_dir, these lines still create the org file, state directory, and state file before the later if not dry_run guard. That contradicts the advertised dry-run behavior and can
leave files behind even though the user asked to only preview actions. -
[P2] Compare properties symmetrically — /home/barts/code/personal/python/khalorg/src/khalorg/org/agenda_items.py:312-316
When a property exists only on b, this loop never sees it, so OrgAgendaItem.compare(a, b) can return true even though b.properties has extra data. In sync this means removing a property from the org file can be treated as unchanged
against the state item, so the removal is skipped and never pushed to khal.
BartSte
left a comment
BartSte
left a comment
There was a problem hiding this comment.
Posted this review for codex 🤖.
2 participants
Hi @BartSte , I've been developing today the
syncfeature.While i was at it i've also added some features:
similarmethod to OrgAgendaItemCorrected some bugs:
fix: correct day OrgRegex to work with latin locales
fix: set the default logger level to INFO
fix: avoid mutable arguments in checker
It was polluting the state between tests
fix: prevent duplicate timestamps when start == end
And done other small improvements:
Sorry if the accumulation of changes makes the PR difficult to read :(
Closes #3