V1.2.0 dev - refactoring, bugfixes, performance#43
Draft
tobixen wants to merge 17 commits into
Draft
Conversation
The config handling code in plann was adopted by the caldav library a while back, but plann kept its own copy. Drop the duplicates: - read_config, config_section and expand_config_section in plann/config.py now come from caldav.config (read_config through a thin wrapper preserving plann's log-and-continue behaviour on a broken config file) - find_calendars delegates connection parameter extraction and the calendar lookup to caldav.config.extract_conn_params_from_section and caldav.get_calendars This also fixes a crash (KeyError: 'url') for config sections that carry a `features` server profile but no caldav_url - the caldav library derives the URL from the profile's auto-connect hints (since v2.2.1) prompt: I have this configuration in ~/.config/calendar.conf: [ecloud section with caldav_pass, caldav_user, features but no URL]. The URL should not be needed [...] This is also reproducible by using plann: KeyError: 'url' followup-prompt: It's needed to fix the bug in the caldav library obviously, but plann also needs some refactoring, it should trust the caldav library to do things right instead of carrying a separate code path for this logic. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
find_calendars attached the section's extra config to the calendar
objects as `extra_params` (sourced from a `caldav_extra_params` key),
while add_time_tracking read `obj.parent.extra_config` - so a
`"extra_config": {"time_tracking": [...]}` config section never had
any effect. Attach it as `extra_config`, always (defaulting to an
empty dict), sourced from the unprefixed `extra_config` key.
Also port two more time tracking fixes from the archived development
branch (commit 8c9e742):
- accept "timewarrior" and "Timewarrior" in addition to "timew" (the
error message told the user to configure `timewarrior`, which was
not accepted)
- don't pop the categories off the icalendar component when exporting
to timewarrior
prompt: time tracking hacks have been working. Please ensure they
will still work.
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Salvage the two genuinely-novel ideas from the archived/development branch (commit "what's this?"); the rest of that branch had already been reimplemented on master (config delegation, time tracking). * `list --separator=...` joins listed items with a configurable string (defaults to a newline), threaded through `_list`. * The interactive edit prompt now advertises the `start` command and re-prompts afterwards so a follow-up command can be given for the same task. `command_edit` already handled `start`; it just wasn't reachable from the menu. prompt: Please add the --separator feature and reproduce the interactive 'start' in the current branch. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Fixes from docs/code-review-2026-06-12.md (bug #13 on hold, #6/#7/#8 via relativedelta): - cli.py: fix --startnow/--track missing leading dashes (#1) - cli.py: fix all(isinstance(...)) guard in add-time-tracking (#9) - cli.py: add type=int to --limit on check-due command (#11) - cli.py: stop double-prefixing lookahead with '+' in dismiss_panic (#15) - commands.py: fix discarded children result, undefined 'parents', and icalendar_comp typo in --no-pinned-tasks Todo branch (#2) - commands.py: prefix lookahead in exactly one place (_dismiss_panic) (#15) - interactive.py: fix inverted condition in interactive_split_task postpone prompt (#5) - lib.py: dedent return in _relationship_text so all relation types are shown, not just the first (#3) - lib.py: apply filter to first object too in _list(ics=True) (#4) - lib.py: fix icalendar_component_UID attribute access in error log (#12) - lib.py: wrap scalar time_tracking string in list before iterating (#14) - panic_planning.py: guard get_dtend() result for None/date before comparing with aware datetime (#10) - timespec.py: fix year constant (1314000 → 31536000), use dateutil.relativedelta for year arithmetic (handles date vs datetime and Feb-29), accumulate diff across loop iterations (#6, #7, #8) - pyproject.toml: add python-dateutil as explicit dependency prompt: look into docs/code-review-2026-06-12.md and start fixing things followup-prompt: do it as suggested (re: using dateutil.relativedelta for year branch) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- C1: delete stale _interactive_edit from commands.py; import canonical version from interactive.py which has the 'start' time-tracking command and re-prompts after it — the stale copy lacked both - C2: extract _pdb_edit(obj) in interactive.py; replace duplicated 5-line pdb setup block in commands.py and command_edit with a single call - C3: interactive_split_task used inline summary fallback that duplicated _summary; replace with _summary(obj) - C4: delete get_obj closure in _set_relations_from_text_list; use _get_obj_from_line (which handles comment stripping and edge cases more robustly); also guard against None return when appending children prompt: Deal with "duplication and drift" Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…uage tests
Export DURATION_UNITS / DURATION_TOKEN_RE / DURATION_RE / is_duration() from
timespec.py and use them in the four sites that previously each hard-coded the
[smhdwy] unit set (parse_add_dur, _parse_timespec, commands.__select,
interactive._command_line_edit). Adding a new unit (e.g. months) is now a
one-line change. Marks C5 fixed in docs/code-review-2026-06-12.md.
Add explicit tests that natural-language dates ("yesterday", "in 2 days") are
accepted, including through parse_timespec(), plus a TestDurationGrammar class.
Also remove a pre-existing unused command_edit import in commands.py so
`ruff check plann/` is clean.
Note: date parsing already delegates to dateparser (commit 0ea94fb); the
compact [smhdwy] duration grammar stays local because dateparser parses dates,
not durations (relativedelta handles the calendar arithmetic).
prompt: In many other scripts and apps I've made, I'm using the dateparser library to parse dates. There is an ongoing code review under docs/code-review-2026-06-12 that comments about bugs and issues with the date parsing. Please have a look if we can use dateparser instead of local parsing. Also, fix the issues if they will still be relevant after introducing dateparser.
followup-prompt: but make explicit test that dates like "yesterday" is accepted, and also check the "Duration grammar in four places" from the code review, it's marked as not fixed
followup-prompt: ruff errors should be fixed even if they are pre-existing
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
C6: centralise component-type detection. Add _component_type() and _caldav_objclass() to lib.py and replace the scattered 'BEGIN:VEVENT' in obj.data raw-string sniffing (panic_planning.py, interactive.py, commands.py, cli.py). The old approach also matched the substring inside a description/summary body; the helpers parse the component name instead. C10: _editor() now uses shutil.which() instead of a hand-rolled PATH search, and raises a clear FileNotFoundError when no editor is found instead of leaving a non-executable fallback bound. D1: remove unreachable `raise NotImplementedError` at end of _parse_timespec. D2: `command_edit` checked `'with family'` twice; the second was meant to be `'with parent'` so `postpone 1d with parent` silently did nothing. prompt: continue picking items from docs/code-review-2026-06-12.md Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The plural-vs-singular category special-casing was spread across several sites (_process_set_arg, _set_something, _add_category, the cli option generator and _set_task_attribs). Introduce a single lib.COMMA_LIST_ATTRS registry plus helpers (_is_comma_list_attr / _comma_list_canonical / _comma_list_is_plural / _comma_list_tokens / _add_comma_list / _set_comma_list) and have the edit path delegate to them. Generalised to RESOURCES while there: edit now exposes --add-category, --add-categories, --add-resource and --add-resources (plural splits on comma, singular keeps it literal), and --set-resources now splits on comma like --set-categories always has. --set-category is kept for backwards compatibility but flagged deprecated in its --help (it appends, which the name does not convey). The select-by --category (substring) / --categories (exact) distinction is intentional - it maps to the caldav library's search semantics - and is left untouched. Also fixes the assert-less no-op checks in test_add_set_category and gives test_plann_cli.py its first real tests. prompt: Now go back to the original plann C7 followup-prompt: (earlier decisions) comma-lists only for categories+resources; expose both singular and plural for add/set so the user need not guess; keep the --add-category comma-literal quirk; no --set-resource; --set-category deprecated in --help only Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
C8 flagged interactive_config for hardcoding a config-key list that had diverged from what the caldav reader actually consumes. While fixing it, discovered the function was dead code: orphaned during the argparse->click migration (it still expected an argparse `args` namespace and had no caller). - Re-wire as a `plann configure` click subcommand; refactor the function to take explicit (config, config_file, config_section, allow_use) params. - Derive the connection prompt keys from caldav.config.CONNKEYS with an assertion that they map to real connection parameters, so the writer can no longer silently drift from the reader. - Fix `ssl_verify_cert` -> `caldav_ssl_verify_cert` (the bare key was silently dropped by caldav's extractor), add `features`, `calendar_name` and `extra_config.time_tracking`, drop never-read `language`/`timezone`. - Strengthen the runtime "here be dragons / under-tested" disclaimer. - Tests: round-trip the written connection keys through the caldav extractor and assert the configure command is registered. prompt: docs/code-review-2026-06-12.md - deal with C8. followup-prompt: re-wire, fix, but make sure to print out a disclaimer that the interactive configuration is under-tested and that there may be dragons. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…1, E1) C11: _split_vcals scanned the raw string by hand at a fixed 14-char offset, assuming LF line endings - so concatenated VCALENDAR streams using the RFC 5545 canonical CRLF endings split into nothing. Delegate to icalendar.Calendar.from_ical(ical, multiple=True), which handles CRLF and line folding. Implemented the previously-stubbed test_split_vcals plus a CRLF regression test. E1: the cli() group ran find_calendars() unconditionally, so even `plann <subcommand> --help` connected to every configured calendar. Wrap discovery in a _LazyCalendars list-like that resolves on first use, and fold the command-line + config-file discovery into a single _discover_calendars(). prompt: Continue with C-11 and E-1 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… E2)
The '{...}' sort-key branch built a fresh Template(skey) inside the sort key
function, so it was recompiled on every comparison (O(n log n) times) instead
of once per sort key. Extract the sort-key logic into _sort_key_function,
compiling the template once; this also removes the closure-in-loop pattern and
is independently testable.
prompt: Fix E2
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`list --top-down`/`--bottom-up` walks the parent/child graph and re-fetched the same task from the server once per edge: _relships_by_type fetched every relative via get_object_by_uid, and the _list recursion re-ran that for each node, so a parent was fetched anew for every child - ~N×R round-trips. Add a per-traversal _RelativeCache that memoizes object-by-UID lookups and the per-object relationship scan. _relships_by_type now parses the related UIDs locally (get_relatives(fetch_objects=False), no network) and resolves them through the cache; the cache is threaded through the _list recursion. This is caching, not opt-in: the relationship scan is still always done for the hierarchical view (it's needed to build the tree), just without the redundant fetches. prompt: E3. Making it "opt-in" doesn't sound like a fix, I think it's needed to know about relationships when doing a "top down" hierarchical task list. Please consider and come with suggestions Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
_set_task_attribs ran a fresh server select for each of category/due/priority/ duration (plus another to list categories in use) - ~5 round-trips, each re-fetching the task list, even though the code already re-filtered the results client-side. Fetch the pending todos once and filter client-side per attribute using the icalendar_searcher library's `undef` operator - the same semantics caldav uses for the server-side no_<x> search. The category branch reuses the same fetch. 5 round-trips collapse to 1. Side benefit: on servers that do not honour the no_<x> filter, the previous code applied the server-side limit before client-side filtering and so could miss tasks; filtering the full fetch finds them. icalendar_searcher is declared as an explicit dependency (it was already present transitively via caldav). prompt: E4 - there is the library icalendar_searcher (an implicit dependency already as it's used by caldav) that can do client-side filtering of icalendar data Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The code review suggested stopping the per-calendar UID lookup at the first hit. That would silently drop the case where the same UID exists in more than one calendar (e.g. a copied event/task), which --uid deliberately collects. The U×C lookups are inherent to "find this UID in any calendar" (there is no batch-by-UID query). Document the intent in the code so it is not "optimised" into a bug, and record the rationale in the review table. prompt: I'm not sure if E5 is something that should be "fixed". the fix is basically to stop iterating over calendars as soon as a matching object is found? Now, what if the object exists in several calendars with the same uid? Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`plann select --uid=asdf delete` previously produced no output at all, whether or not the UID matched anything. delete now names each item as it is deleted and prints "No items selected for deletion" on an empty selection, so the two cases are distinguishable. Ref #42 prompt: Work on issue #42 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Generalises the issue #42 feedback gap beyond delete: a --uid that is not found in any calendar is no longer silently ignored. By default __select now warns (to stderr) naming the missing uid(s). --abort-on-missing-uid still takes precedence; --no-warn-on-missing-uid restores the old fully-silent behaviour. Ref #42 prompt: let's make a more general solution: introduce --warn-on-missing-uid/--no-warn-on-missing-uid (or something like that) with warn being the default Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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.
I was almost able to run a full code review using Claude Fable on this project before the model was yanked - two subagents was killed mid-flight, I had to let Opus take over. Anyway, a lot of issues were found. I've looked through all of them and had the AI fix up most of them. So v1.1.2 is mostly about refactoring, performance tuning and bugfixes.
Except, two minor features that I forgot committing a year ago or so has been included. One of them is a quite niche feature only used by my own workflow - in interactive mode it's possible to start time-tracking using the Timewarrior tool. The other is a command line option to change the delimiter (default is newline) in the list mode. (I did need it for something, but I don't remember what - but by combining
--separatorand--templateit's possible to do things like getting a comma-separated list of UIDs from the tool).Be aware that this heavy round of refactoring gives a risk of new bugs introduced.
The very most of the work here was done by Claude Opus, some with Claude Sonnet. I have to do quite some manual QA on this before removing the "draft"-status.