diff --git a/.github/workflows/linkcheck.yml b/.github/workflows/linkcheck.yml index c5e6987..513b744 100644 --- a/.github/workflows/linkcheck.yml +++ b/.github/workflows/linkcheck.yml @@ -2,11 +2,20 @@ name: Link check on: push: + branches: + - master pull_request: workflow_dispatch: schedule: - cron: "03 22 * * *" +# Serialise runs that share a ref so two link-check runs never race to create +# the "Link Checker Report" issue. (The old setup let a push and the matching +# pull_request run both file an issue at the same second - cf. issues #44/#45.) +concurrency: + group: linkcheck-${{ github.ref }} + cancel-in-progress: false + jobs: linkcheck: runs-on: ubuntu-latest @@ -26,29 +35,43 @@ jobs: with: fail: false args: >- - --config .lychee.toml - --timeout 20 - --max-retries 3 + --config lychee.toml + --timeout 30 + --max-retries 6 + --retry-wait-time 2 --cache --max-cache-age 14d . + + # Issue management only happens on the default branch (scheduled run, push + # to master, or manual dispatch). PR and feature-branch runs only surface + # the lychee output in the job log: this avoids issue noise from transient + # branch states and removes the duplicate-issue race entirely, since only + # master-ref runs touch issues and the concurrency group serialises those. - name: Create or update Link Checker issue - if: steps.lychee.outputs.exit_code != 0 + if: steps.lychee.outputs.exit_code != 0 && github.ref == 'refs/heads/master' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | - EXISTING=$(gh issue list --label "report" --state open --json number --jq '.[0].number' 2>/dev/null || echo "") - if [ -n "$EXISTING" ]; then - gh issue edit "$EXISTING" --body-file ./lychee/out.md + # Keep the oldest open report issue as the canonical one and fold any + # duplicates into it, so accumulated dupes self-heal over time. + ISSUES=$(gh issue list --label "report" --state open --json number --jq '.[].number' | sort -n) + CANON=$(printf '%s\n' "$ISSUES" | head -1) + for n in $(printf '%s\n' "$ISSUES" | tail -n +2); do + gh issue close "$n" --comment "Duplicate of #${CANON} - auto-closed by the link checker." + done + if [ -n "$CANON" ]; then + gh issue edit "$CANON" --body-file ./lychee/out.md else gh issue create --title "Link Checker Report" --body-file ./lychee/out.md --label "report" fi - name: Close Link Checker issue if all links are healthy - if: steps.lychee.outputs.exit_code == 0 + if: steps.lychee.outputs.exit_code == 0 && github.ref == 'refs/heads/master' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | - EXISTING=$(gh issue list --label "report" --state open --json number --jq '.[0].number' 2>/dev/null || echo "") - if [ -n "$EXISTING" ]; then - gh issue close "$EXISTING" --comment "All links are now healthy." - fi + # Close every open report issue, not just the first, so stragglers from + # earlier races also get cleaned up. + for n in $(gh issue list --label "report" --state open --json number --jq '.[].number'); do + gh issue close "$n" --comment "All links are now healthy." + done diff --git a/.gitignore b/.gitignore index d6003f1..de6f44b 100644 --- a/.gitignore +++ b/.gitignore @@ -55,6 +55,9 @@ docs/build/ tmp/ *.log +# lychee link checker local cache +.lycheecache + # OS .DS_Store Thumbs.db diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 0f25525..3f73d95 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -61,8 +61,13 @@ repos: stages: [commit-msg] - repo: https://github.com/lycheeverse/lychee - rev: lychee-v0.23.0 + rev: lychee-v0.24.2 hooks: - - id: lychee-docker - args: ["--no-progress", "--timeout", "10", "--config", ".lychee.toml"] - types: [markdown, rst] + - id: lychee + # Check links across the whole working tree (all file types, not just + # md/rst). pass_filenames: false -> pre-commit passes only '.', so the + # whole repo is scanned rather than just the changed files. lychee + # auto-discovers lychee.toml, so no --config needed. + args: ["--no-progress", "--timeout", "10", "--cache", "--max-cache-age", "1d", "."] + pass_filenames: false + stages: [pre-push] diff --git a/CHANGELOG.md b/CHANGELOG.md index c5154a4..78910f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,49 @@ The format of this file is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and I do try to adhere to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Added + +* `edit --add-categories`, `--add-resource` and `--add-resources` options (alongside the existing `--add-category`). A single value containing commas is split by the plural forms (`--add-categories`/`--add-resources`) and kept literal by the singular forms (`--add-category`/`--add-resource`). +* `select ... list --separator=...` to join the listed items with something other than a newline. (Ported from the archived development branch.) +* The interactive edit prompt now advertises the `start` command (kicks off time tracking for the task) and re-prompts afterwards so a follow-up command can be given for the same task. (Ported from the archived development branch.) +* `select --warn-on-missing-uid/--no-warn-on-missing-uid` (default: warn). A `--uid` that matches nothing in any calendar now prints a warning naming the missing uid(s) to stderr, rather than being silently ignored. `--abort-on-missing-uid` still takes precedence, and `--no-warn-on-missing-uid` restores the old fully-silent behaviour. Ref https://github.com/tobixen/plann/issues/42 +* `plann configure`: an interactive configuration mode (EXPERIMENTAL/under-tested) that prompts for connection parameters and writes them to the config file. The underlying code existed but had been orphaned in the argparse→click migration; it is now wired up again, and the prompted keys match what the caldav library actually reads (so e.g. `ssl_verify_cert` is no longer written under a key that is silently ignored at connect time). + +### Fixed + +* `add ical` with several concatenated VCALENDAR objects no longer silently ignores them all when the data uses CRLF line endings (the RFC 5545 canonical form): the split is now done by the icalendar library instead of a hand-rolled LF-only string scan. +* Showing help for a subcommand (e.g. `plann select --help`) no longer connects to every configured calendar; calendar discovery is deferred until a command actually needs it. +* A config section carrying `features` but no `caldav_url` crashed with `KeyError: 'url'`. The caldav library resolves the URL from the server profile given in `features`, so no URL is needed. (This also requires a caldav release newer than 3.2.1 - with older caldav versions such a section is silently skipped instead of crashing.) +* The time tracking integration (`"extra_config": {"time_tracking": ["timewarrior"]}` in a config section) did not work: the configuration was attached to the calendar objects under a different attribute name than the time tracking code read, and only the value `timew` was accepted - not `timewarrior` as the error message suggested. (Fixes ported from the archived development branch.) +* Exporting an event/task to timewarrior no longer removes the categories from the object. +* `select ... delete` now reports what it did: it names each item as it is deleted, and says "No items selected for deletion" on an empty selection instead of silently producing no output regardless of whether anything matched. Ref https://github.com/tobixen/plann/issues/42 +* `select ... add-time-tracking` could not be run at all - it aborted with an "Invalid start character for option" error before doing anything. +* Durations were computed wrongly in several ways: `1y` came out as roughly 15 days rather than a year, and a compound duration such as `1h30m` kept only its last component (30 minutes). Adding a year to a plain date, or to February 29, raised an error. +* `interactive check-due --limit N` crashed instead of limiting the number of tasks shown. +* `select --no-pinned-tasks` crashed when tasks were included in the selection. +* `interactive split` did not postpone the task when asked to - the prompt was inverted, so answering with a duration did nothing and declining postponed it. +* `postpone with parent` in interactive editing silently did nothing. +* The relationship overview showed only the first kind of relation, so e.g. children were listed but parents were not. +* `list --ics` exported the first matching object without applying the filter to it. +* `dismiss-panic` built a malformed timestamp (`++60d`) and failed. +* Checking due dates no longer crashes on all-day events that have relations. +* Reporting an inconsistent relationship crashed instead of logging what was wrong. +* A `time_tracking` setting written as a plain string rather than a list was read one character at a time. +* Postponing a task with parents could drop the user into the Python debugger. +* `edit` now reports clearly that no editor could be found, rather than failing in a confusing way further down. + +### Changed + +* `edit --set-resources` now splits its value on comma, the same way `--set-categories` always has; `--set-resources a,b` now sets two resources rather than one resource literally named `a,b`. +* `edit --set-category` is now flagged as deprecated in its `--help`: it *appends* rather than replaces (the name does not convey this). Use `--add-category` to append or `--set-categories` to replace. +* (internal) The `category`/`categories` (and now `resources`) handling on the edit path is driven by a single `COMMA_LIST_ATTRS` registry rather than being special-cased in several places. +* (internal) A template sort key (`--sort-key`) is now compiled once per key instead of being rebuilt on every comparison while sorting. +* (internal) A hierarchical `list --top-down`/`--bottom-up` now caches related tasks for the duration of the traversal instead of re-fetching the same task from the server once per relationship edge. +* (internal) `set-task-attribs` now fetches the task list once and filters client-side per attribute (via the `icalendar_searcher` library) instead of issuing a separate server query for each of category/due/priority/duration. As a side effect, on calendar servers that do not filter properly it now finds tasks missing an attribute that it previously could miss. +* Config file parsing, connection parameter extraction and calendar lookup are now delegated to the caldav library instead of being duplicated in plann. (The caldav library adopted this code from plann a while back; plann was still carrying its own copy.) Visible side effects: environment variable references like `${SOME_VAR}` or `${SOME_VAR:-default}` in config values are now expanded, and a `features` key is resolved through the caldav library's profile lookup. + ## [v1.1.1] - 2026-05-28 ### Added diff --git a/README.md b/README.md index ca68ae5..be1f969 100644 --- a/README.md +++ b/README.md @@ -90,6 +90,7 @@ The caldav URL should be something like i.e. http://some.davical.server/caldav.p The list may not be complete. `--help` should give a more complete overview. * list-calendars: lists the calendars that plann can see +* configure: interactive configuration mode (EXPERIMENTAL/under-tested) - prompts for connection parameters and writes them to the config file * agenda: Convenience command, lists upcoming events and tasks * add: adds new events/items to todo lists/calendars * select: select/search/filter tasks/events to list/modify/mark complete and so forth @@ -142,6 +143,11 @@ The file may look like this: } } ``` + +The file can be written by hand, or generated interactively with `plann configure` (experimental/under-tested - here be dragons). + +A `features` key may be given to enable server-specific compatibility workarounds, referring to a server profile in the caldav library's `compatibility_hints` module (e.g. `"features": "ecloud"`). For servers with a known profile, `caldav_url` may even be omitted - the caldav library will derive the URL from the profile. + A configuration with multiple sections may look like this: ```json diff --git a/TASK_MANAGEMENT.md b/TASK_MANAGEMENT.md index 12cfeb7..db284b3 100644 --- a/TASK_MANAGEMENT.md +++ b/TASK_MANAGEMENT.md @@ -128,7 +128,7 @@ While the categories field is a freetext field, it's important that the same cat My usage of categories may be slightly superceded by "concept", "link" and "refid", as defined in RFC9253. I should look into that and consider if it's useful for plann. -After some thinking, I've considered that quite much of what I use "categories" for would possibly be more appropriate to put in the "resources"-field. "Good weather" may be considered as a resource rather than a category, "keyboard" may be considered a resource, "supermarked" may be considered to be a resource. When having a certain set of resources available it makes sense to do as many tasks as possible with the given set of resources. Resources may be missing, then the alternatives are to find the missing resources (or travel to them ... or try to make without them) or to postpone the task until the resources are available. Plann has no specific support for resources, but I should consider it. +After some thinking, I've considered that quite much of what I use "categories" for would possibly be more appropriate to put in the "resources"-field. "Good weather" may be considered as a resource rather than a category, "keyboard" may be considered a resource, "supermarked" may be considered to be a resource. When having a certain set of resources available it makes sense to do as many tasks as possible with the given set of resources. Resources may be missing, then the alternatives are to find the missing resources (or travel to them ... or try to make without them) or to postpone the task until the resources are available. Resources can now be edited just like categories (`edit --add-resource`/`--add-resources`/`--set-resources`), but plann has no resource-aware planning logic yet. RFC9073 also defines vresource, which is a more structured way of specifying resources. @@ -313,7 +313,7 @@ Those are not much relevant wrg of task handling in plann ## Daily task management and interactive mode -This is specifically directed towards using plann for daily task management. See also the [USER GUIDE](USER_GUIDE.md) for more generic user guide. I will assume that your calendar server needs to support advanced CalDAV queries (see [CALENDAR SERVER RECOMMENDATIONS](docs/ALENDAR_SERVER_RECOMMENDATIONS.md)). +This is specifically directed towards using plann for daily task management. See also the [USER GUIDE](USER_GUIDE.md) for more generic user guide. I will assume that your calendar server needs to support advanced CalDAV queries (see [CALENDAR SERVER RECOMMENDATIONS](docs/CALENDAR_SERVER_RECOMMENDATIONS.md)). ### Adding tasks diff --git a/USER_GUIDE.md b/USER_GUIDE.md index 7790763..ad82da4 100644 --- a/USER_GUIDE.md +++ b/USER_GUIDE.md @@ -32,7 +32,7 @@ Those commands are made mostly for making `plann` more convenient to use for the * interactive manage-tasks - go through your tasks and make suggestions * interactive update-config - (TODO: NOT IMPLEMENTED YET). This one is not used by the primary author and is probably under-tested. Its primary intention is to make it easy for others to use the tool. -Note that many of those commands have only been tested on DAViCal (see the [`docs/ALENDAR_SERVER_RECOMMENDATIONS.md`](CALENDAR_SERVER_RECOMMENDATIONS.md) file) +Note that many of those commands have only been tested on DAViCal (see the [`docs/CALENDAR_SERVER_RECOMMENDATIONS.md`](docs/CALENDAR_SERVER_RECOMMENDATIONS.md) file) ## Global options diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md new file mode 100644 index 0000000..29959bf --- /dev/null +++ b/docs/ROADMAP.md @@ -0,0 +1,445 @@ +# Roadmap: plann + +**This document is a draft** - it was generated by Claude Opus 5.0. As of 2026-08-25 significant parts of it has been reviewed and edited by Tobias Brox. In any case, **an estimate is an estimate** - the only way to know for sure how much time a task will take is to actually do it. + +## Funding + +[NLnet foundation logo](https://nlnet.nl) +   +[NGI Zero Core Logo](https://nlnet.nl/core) + +This project is funded through the [NGI Zero Core](https://nlnet.nl/core) fund, a +fund established by [NLnet](https://nlnet.nl/) with financial support from the +European Commission's [Next Generation Internet](https://ngi.eu/) programme, under +the aegis of DG Communications Networks, Content and Technology. + +The funded scope is 144 hours. This was based on an initial +human-made rough plan of five task buckets. I asked Claude Opus to +make me a roadmap based on the human-made plan, open issues, existing +design documents and the code itself. The buckets have been +redefined, reorganized and reprioritized, improving quality is the top +priority now. Importantly, this document defines deliverables for +every task. + +### How to read this document + +**The issue tracker is the source of truth for task-level detail.** Every item +below names its deliverable and links to its issue; the issue holds the concrete +task list, the code references and the acceptance criteria, and is kept current as +the work proceeds. + +This document holds what an issue tracker is bad at: the shape of the whole, why the +hours are distributed the way they are, what depends on what, what gets released +when, and what is deliberately *not* funded. Where this document and an issue +disagree on a detail, the issue wins. + +--- + +## Overview + +`plann` is a command-line CalDAV client for calendar events, journals and - above +all - task management. It is the successor to +[`calendar-cli`](https://github.com/pycalendar/calendar-cli/), and v1.0 shipped in +December 2024. + +The tool works, and the author uses it daily. What it is not, yet, is a project +other people can pick up without reading the source: the test suite covers 50% of +the code (22% of `commands.py`, the module that holds most of the logic), the +package carries no type annotations at all, several advertised commands raise +`NotImplementedError`, and the user guide predates the 1.0 release and says so in +its own first paragraph. + +The funded period is therefore weighted towards **making the existing tool +trustworthy** rather than towards new features - with one substantial exception +(Phase 4), which builds out the time-tracking and reporting story that +[`NEXT_LEVEL.md`](../NEXT_LEVEL.md) has argued for since the beginning and which the +code has never actually delivered. + +### Guiding constraints + +1. **Correctness before surface area.** The 2026-06 code review found fifteen + correctness bugs, several of which made whole subcommands unreachable. They are + fixed, and ship in v1.2.0 before this funded period begins. What the review also + showed is *why* they survived so long: the code they lived in had almost no + tests. Fixing the bugs without fixing that leaves the next fifteen in place. +2. **plann is a CLI, not a library.** [`DESIGN.md`](../DESIGN.md) states that plann + "should be a simple command line wrapper over existing python libraries" and that + reusable logic belongs downstream. The codebase has drifted from this - relation + handling, duration parsing and search logic all live in `plann/lib.py`. Phase 3 + is the correction, not an aesthetic preference. +3. **Don't reinvent, and don't hoard.** `caldav`, `icalendar`, + `icalendar-searcher`, `recurring_ical_events` and (soon) `calendaring-client` + are maintained by the same group of authors. Bugs found in them get fixed + there, on their budget, not worked around here. +4. **The task-management opinions are the product.** + [`TASK_MANAGEMENT.md`](../TASK_MANAGEMENT.md) and [`NEXT_LEVEL.md`](../NEXT_LEVEL.md) + contain a worked-through theory of how to abuse RFC 5545 into a usable task + tracker. That theory is what distinguishes plann from `khal` and from every + other CalDAV CLI. Documentation work (Phase 5) is therefore not garnish. +5. **An honest changelog and a real release cadence.** v1.1.0 shipped broken + (the publishing workflow did not work) and v1.1.1 shipped the same day. A + release that users can install is the unit of delivery, and every phase below + is scoped so that it ends in one. + +--- + +## Phase 1: Bug fixing and user-facing gaps (20 hours) + +**Baseline:** v1.2.0 ships *before* this funded period begins and is not charged to +it. It carries the fifteen correctness bugs from the June 2026 code review, roughly +twenty other fixes and the config-delegation refactor - everything currently in the +`[Unreleased]` section of [`CHANGELOG.md`](../CHANGELOG.md) and in +[PR #43](https://github.com/pycalendar/plann/pull/43). Everything below is measured against that +release, not against the current master. + +What remains are the things a user hits and the author does not: two open reports +against Office 365 through a DavMail gateway, a mass-edit path that is wrong across +calendars, and a configuration story that only works if you already have a working +config file. + +| Item | Deliverable | h | € | Issue | +|---|---|---|---|---| +| 1.1 CalDAV server compatibility | Fixes, or a documented verdict that the fault is `caldav`'s or the server's, with issues filed there | 8 | 400 | [#25](https://github.com/pycalendar/plann/issues/25), [#26](https://github.com/pycalendar/plann/issues/26) | +| 1.2 Multi-calendar mass-edit | Mass-editing that is correct across calendars, or refuses to run when it cannot be | 4 | 200 | [#48](https://github.com/pycalendar/plann/issues/48) | +| 1.3 Improving the configuration helper logic | A working `interactive update-config`, a `plann configure` without its "here be dragons" warning, and documented non-plaintext credential options | 4 | 200 | [#49](https://github.com/pycalendar/plann/issues/49) | +| 1.4 Downstream contributions | Outstanding community contributions merged or answered | 4 | 200 | [PR #46](https://github.com/pycalendar/plann/pull/46) | + +1.1 carries a dependency the project cannot satisfy on its own: the author has no +Office 365 account and no DavMail experience, so it needs the original reporter or +another affected user. If that help does not materialise, the hours move to Phase 2. + +1.3 is deliberately the *command-line* half only. Credential storage belongs in the +library - see the scope note in [#49](https://github.com/pycalendar/plann/issues/49), and constraint 2 above. + +--- + +## Phase 2: Quality assurance (48 hours) + +The largest block by a wide margin, and the one the project most needs. 98 tests +pass at **50% statement coverage**, with the modules holding the actual logic covered +worst: + +| Module | Lines | Cov | | Module | Lines | Cov | +|---|---|---|---|---|---|---| +| `commands.py` | 456 | **22%** | | `config.py` | 105 | 61% | +| `interactive.py` | 266 | **36%** | | `panic_planning.py` | 119 | 71% | +| `lib.py` | 396 | 62% | | `template.py` | 42 | 84% | +| `cli.py` | 353 | 64% | | `timespec.py` | 132 | 88% | + +Every one of the fifteen correctness bugs found in June 2026 lived in code this thin +on tests, and several were *silent* wrong behaviour that no user would have reported. +Beyond coverage: **zero** type annotations across 139 function definitions, and +**88** `TODO`/`FIXME` comments, two of which turned out to be unreported bugs when +read during this review. + +| Item | Deliverable | h | € | Issue | +|---|---|---|---|---| +| 2.1 Coverage for `commands.py` / `interactive.py` | Both modules at or above 70%, with a CI coverage floor so it cannot drift back | 16 | 800 | [#50](https://github.com/pycalendar/plann/issues/50) | +| 2.2 End-to-end command-line tests | An executable suite exercising the documented command lines against a real server | 10 | 500 | [#13](https://github.com/pycalendar/plann/issues/13) | +| 2.3 Type annotations and static checking | Annotated public functions, mypy running clean in CI | 12 | 600 | [#51](https://github.com/pycalendar/plann/issues/51) | +| 2.4 TODO triage | TODO count reduced to genuinely local notes; everything else an issue or deleted | 6 | 300 | [#52](https://github.com/pycalendar/plann/issues/52) | +| 2.5 Remaining code-review debt | The last open item (`C9`) from the June 2026 review closed | 4 | 200 | [#53](https://github.com/pycalendar/plann/issues/53) | + +2.2 is also what keeps Phase 5 honest: a user guide whose examples run in CI cannot +rot. 2.5 blocks Phase 4 - per-calendar configuration that vanishes depending on how +the object was fetched cannot carry a reporting feature. + +--- + +## Phase 3: Migration to calendaring-client (16 hours) + +`calendaring-client` is a general Python client library for calendaring **and task** +data with one API across CalDAV, JMAP, local `.ics` files, iCalendar feeds and task +trackers. plann is named in its roadmap as the designated dogfooding consumer, and +multi-backend support - notably issue trackers - has been on plann's own wish list +since [`DESIGN.md`](../DESIGN.md) was written. + +1.3 and 4.1 both concluded that logic they used to own belongs in the library rather +than in plann. That does not make the work disappear - it makes plann a *consumer* +of an API that has to exist. Rather than budget that here, **600 EUR (12 hours) was +moved from this grant to the `calendaring-client` grant**, which now carries an +explicit "Time-tracking model and API" item alongside its existing configuration and +credentials item. This roadmap therefore totals 144 hours rather than 156, and the +library work is funded where it will be written. + +**These 16 hours do not buy a completed migration.** `calendaring-client`'s roadmap +estimates the full port at 24-40 hours and places it outside its own funded scope. + +| Item | Deliverable | h | € | Issue | +|---|---|---|---|---| +| 3.1 Port plann onto calendaring-client | A meaningful subset running on the new library, one non-trivial command proven against a non-CalDAV backend, and a written API critique delivered before its 1.0 freeze | 16 | 800 | [#55](https://github.com/pycalendar/plann/issues/55) | + +--- + +## Phase 4: Time tracking and milestone reporting (18 hours) + +plann's distinguishing claim - that a calendar is the right place to keep project +management and accounting data - is currently a claim in a design document and almost +nothing in the code. `sum_hours` raises `NotImplementedError`, and `complete` has +none of the `--spent`/`--log`/`--start`/`--end` parameters +[`DESIGN.md`](../DESIGN.md) specifies in detail. The only working time tracking +shells out to `timewarrior` and stores nothing in the calendar. + +The phase is smaller than the original plan because two things moved out of it: the +iCalendar representation of "time spent" is library work (Phase 3), and the milestone +report is an export format over an aggregated task list rather than a feature of its +own. + +| Item | Deliverable | h | € | Issue | +|---|---|---|---|---| +| 4.1 Time tracking on the command line | `complete --spent/--log/--start/--end`, writing through the library's time-tracking API | 6 | 300 | [#54](https://github.com/pycalendar/plann/issues/54) | +| 4.2 `sum_hours` and aggregation | A working `sum_hours` that groups a selection into budget lines and produces per-line subtotals, keeping committed and spent time separate | 8 | 400 | [#56](https://github.com/pycalendar/plann/issues/56) | +| 4.3 Milestone reporting | An NLnet-format hours/budget export, and a generic invoice-style one, over 4.2's output | 4 | 200 | [#61](https://github.com/pycalendar/plann/issues/61) | + +--- + +## Phase 5: Documentation (42 hours) + +plann has roughly 1,900 lines of Markdown across seven top-level documents, and they +disagree with each other and with the code. [`USER_GUIDE.md`](../USER_GUIDE.md) opens +by admitting it "was written before the 1.0-release, so the actual interface may have +changed a bit". [`DESIGN.md`](../DESIGN.md) documents a `pin` subcommand that does +not exist. The README points at [`NEXT_LEVEL.md`](../NEXT_LEVEL.md) as the roadmap, +and that document opens "This document is dedicated to my rubber ducky". There is a +Sphinx skeleton in `docs/source/` that builds nothing, and `plann.no` is advertised in +`pyproject.toml` as both homepage and documentation. + +| Item | Deliverable | h | € | Issue | +|---|---|---|---|---| +| 5.1 Structure and a published site | A built documentation site, published and built in CI | 12 | 600 | [#57](https://github.com/pycalendar/plann/issues/57) | +| 5.2 Rewrite the user guide | A user guide whose every example is executed in CI | 14 | 700 | [#58](https://github.com/pycalendar/plann/issues/58) | +| 5.3 Task-management method and panic algorithm | The panic-planning algorithm documented for users, and `TASK_MANAGEMENT.md` completed | 8 | 400 | [#59](https://github.com/pycalendar/plann/issues/59) | +| 5.4 Link rot and hygiene | A clean link check, re-run after 5.1 moves files around | 2 | 100 | [#47](https://github.com/pycalendar/plann/issues/47) | +| 5.5 Peer review (reviewers wanted) | Revisions from at least two reviewers who are not the author | 6 | 300 | [#60](https://github.com/pycalendar/plann/issues/60) | + +5.3 matters more than its size suggests: panic planning is plann's most distinctive +feature and its least explicable one. `TASK_MANAGEMENT.md` ends on "TODO: write more +about the panic planning algorithm", and +[`docs/panic_algorithm.md`](panic_algorithm.md) is eighteen lines of notes to a rubber +duck. + +--- + +## Release plan + +| Release | Contents | After | +|---|---|---| +| 1.2.0 | *(baseline - ships before the funded period, not charged to it)* | - | +| 1.3.0 | Server-compatibility fixes, multi-calendar fixes, usable configuration, community patches | Phase 1 | +| 2.0.0 | calendaring-client backend | Phase 3 | +| 2.1.0 | Time tracking in the calendar, `sum_hours` | 4.1, 4.2 | +| 2.2.0 | Milestone reporting | 4.3 | +| 2.3.0 | Documented, typed, covered | Phases 2, 5 | + +The 2.0 number is reserved for the `calendaring-client` migration because that is +where a backward-incompatible change is most likely - and because +[`CHANGELOG.md`](../CHANGELOG.md) commits to "not breaking backward compatibility +unless I really have to (and then under a 2.0-release)". + +Note what the reordering costs: the migration now gates every release after 1.3.0. +If Phase 3 slips (see Risks), Phases 2 and 5 still ship - as 1.4.0 - but time +tracking and reporting do not ship at all. + +--- + +## Effort summary + +Per-item hours are in the phase tables above. + +| Phase | Hours | EUR | Share | +|---|---|---|---| +| 1 - Bug fixing and user-facing gaps | 20 | 1000 | 14% | +| 2 - Quality assurance | 48 | 2400 | 33% | +| 3 - Migration to calendaring-client | 16 | 800 | 11% | +| 4 - Time tracking and milestone reporting | 18 | 900 | 13% | +| 5 - Documentation | 42 | 2100 | 29% | +| **Total** | **144** | **7200** | | + +### Changes from the original five-bucket plan + +The original plan was 156 hours (7800 EUR). It is now **144 hours (7200 EUR)**: 600 +EUR was moved to the `calendaring-client` grant to fund the library work that 1.3 and +4.1 shed. The total across the two grants is unchanged. + +| Original bucket | Was | Now | Why | +|---|---|---|---| +| Bug fixes and downstream improvements | 12 | 20 | Two open user-reported server-compatibility bugs, an unmerged community PR, a multi-calendar data-integrity bug found during this review, and the configuration/credentials path - which is where a new user meets plann and where an advertised command raises `NotImplementedError`. The v1.2.0 release itself is **not** charged here: it ships before the funded period begins | +| QA | 48 | 48 | Unchanged in the end, and still the largest bucket by a wide margin: coverage is 50%, there are no type annotations at all, and two of the 88 TODO comments turned out to be unreported bugs. It was briefly raised to 54 before 600 EUR moved to the sibling grant | +| Migration to calendaring-client | 16 | 16 | Unchanged in size, but moved ahead of the time-tracking work, which now depends on it. The library-side APIs that plann became a consumer of are funded in the `calendaring-client` grant rather than here. It buys a partial port and a design critique, not a completed migration | +| Milestone reporting | 32 | 18 | The iCalendar representation of "time spent" is library work, not plann's. And the report itself is an export format over an aggregated task list rather than a feature of its own - once the aggregation groups a selection into budget lines and totals each one, the rendering is a template | +| Documentation | 48 | 42 | Six hours moved to Phase 1. Documentation is still the second-largest block | + +--- + +## Risks + +### Phase 3 depends on a sibling project that does not exist yet - and now Phase 4 does too + +`calendaring-client` currently consists of a README and a roadmap. No code has +been written. Its own funded scope is 216 hours and its CalDAV backend lands at +roughly hour 100 of that plan. + +This is the largest scheduling risk in this roadmap. The two projects' estimates +for the same work also disagree: `calendaring-client`'s roadmap budgets 6 hours for +its side of the port while estimating the full migration at 24-40 hours in its +"beyond the funded scope" table. + +That disagreement is an artefact of the two plans having been written months apart, +not a structural problem. **Neither roadmap is final, and `calendaring-client` has +not started, so it can still be reprojected** - including moving hours from this +grant to that one where the work turns out to belong there. See the scope note +below. + +**This risk grew when time tracking was made to depend on the migration.** The +migration was originally last and load-bearing for nothing; it is now a prerequisite +for all 18 hours of Phase 4. That puts **34 hours - 24% of the grant** behind an +external project that has not yet written a line of code, and it removes the release +valve: dropping the migration now drops time tracking and milestone reporting with +it. + +**Mitigation:** Phase 3 is scheduled as early as its dependency allows, and Phases 1, +2 and 5 (110 hours, 76%) are deliberately independent of it, so the grant keeps +delivering while it is blocked. Its own deliverable stays defined as *evidence and a +critique* rather than a finished port, so partial delivery is still delivery. + +**Contingency, to be decided rather than discovered:** if `calendaring-client` is not +usable once Phases 1 and 2 are done, Phase 4 is implemented against the `caldav` +library as it stands and ported afterwards. That costs rework, but 4.1's core +question - how to represent time spent in iCalendar - is a data-model decision rather +than a backend one, and the chosen storage format survives the port even when the +plumbing does not. The alternative, waiting, risks the grant ending with neither the +migration nor the feature it now blocks. + +### Work shed upstream needs a home in the upstream budget + +Two items here concluded that logic belongs in the library rather than in plann: +1.3 (configuration and credentials) and 4.1 (the iCalendar representation of time +spent). That is the right call on the merits - see guiding constraint 2 - but it +moves work rather than removing it, and the receiving budget has to actually have +room for it. + +- **Configuration and credentials is already funded upstream.** + `calendaring-client`'s item 1.4 (8 hours) covers config format, discovery, and + "credential handling: keyring integration, and never requiring secrets in + plaintext config". Nothing needs to move; the scope just needs confirming. +- **Time-tracking storage is not.** Only the *research* is funded, inside + `calendaring-client`'s item 0.1, whose task-model survey covers "time estimate and + time spent". There is no implementation line for a time-tracking API, and 4.1 + depends on one existing. + +**Resolved:** `calendaring-client` has been reprojected, with an explicit +"Time-tracking model and API" item (12 h), funded by moving 600 EUR from this grant. +plann's scope drops from 156 to 144 hours accordingly, and the total across the two +grants is unchanged. The work is the same work; which grant pays for it was an +accounting question rather than a technical one. + +The failure mode this avoids is both roadmaps assuming the other is paying - which is +how 4.1 would have ended up blocked on an API nobody had budgeted to write. + +### Milestone reporting has an audience of one + +Phase 4.3 is 9% of the funded period spent on a feature whose most obvious user is +the author, producing reports for this very grant. + +**Mitigation:** the aggregation layer (3.2) is generic and useful on its own, and +the NLnet layout is a template on top of it rather than hard-coded logic. Anyone +who bills by the hour gets the same feature. If it turns out otherwise, the item +is at least self-evaluating: the milestone reports for this grant are produced with +it, so a failure is visible immediately rather than at the end. + +### Time tracking rests on a standard that does not support it + +[`NEXT_LEVEL.md`](../NEXT_LEVEL.md) is explicit that iCalendar has no place to +record time spent: a VJOURNAL cannot carry a DURATION, and the three candidate +workarounds (an illegal DTEND on a journal, `PARTSTAT=X-ATTENDED`, or a new +`VTIMESPENT` component) are all compromises. Whatever Phase 4 picks, other +calendaring tools will not understand it. + +**Mitigation:** the representation is documented (3.1) so that it is at least +*legible* to another implementation, and the `timewarrior` export path is kept so +users who need interoperable time tracking have somewhere to go. This is a known +gap in the standards, not a defect in plann, and saying so plainly in the +documentation is part of the deliverable. + +### The QA phase has no visible output + +Forty-six hours of tests, annotations and TODO triage produce nothing a user can +see. It is the item most likely to be cut when time runs short, and the item whose +absence caused the fifteen correctness bugs that Phase 1 is releasing fixes for. + +**Mitigation:** it is scheduled early, its deliverables are numeric (coverage +thresholds, mypy in CI, TODO counts) rather than subjective, and CI enforcement +means the gains cannot silently erode afterwards. + +### Maintainer bandwidth + +The primary maintainer has several packages to juggle - several of them NLnet-funded - +as well as other priorities and concerns in life. It is a real risk that time will +run out before the allocated funds do. + +**Mitigation:** Phases 1, 2 and 5 are independent of everything else, +so partial delivery is meaningful in any order. Phase 1 alone leaves users better +off than today. There are other developers in the pycal pool who may pick up +pieces. + +### Peer review is a dependency on other people + +Phase 5.5 requires reviewers who are not the author, and the most valuable reviewer +is someone who has never used plann. + +**Mitigation:** approach reviewers during Phase 1, not when the documentation is +ready. + +--- + +## Beyond the funded scope + +Everything below is in plann's ambition - most of it argued for at length in +[`DESIGN.md`](../DESIGN.md), [`NEXT_LEVEL.md`](../NEXT_LEVEL.md) or +[`TASK_MANAGEMENT.md`](../TASK_MANAGEMENT.md) - but **outside the 144 funded hours**. +The maintainer intends to continue developing plann with or without funding, though +the first priority will always be to get food on the table. + +| Item | Rough estimate | Notes | Issue | +|---|---|---|---| +| Creating and editing recurring tasks from the CLI | 16-24 h | `TASK_MANAGEMENT.md` calls recurring tasks "important functionality"; today the RRULE must be written by hand or set from another client. The completion logic already handles them | | +| `pin` - sticking tasks to the calendar | 12-20 h | Fully specified in `DESIGN.md`, never implemented. The conceptual core of the VTODO/VEVENT pairing idea | | +| Loadable module support in `select` | 12-16 h | Arbitrary Python for filtering, editing and ical transformation | [#6](https://github.com/pycalendar/plann/issues/6) | +| Mergecal support - privacy-preserving calendar merging | 12-16 h | Copy busy-time between work and private calendars without leaking detail | [#24](https://github.com/pycalendar/plann/issues/24) | +| RFC 9253 relationship support | 16-24 h | Dependencies and temporal relationships, rather than abusing PARENT/CHILD. `TASK_MANAGEMENT.md`: "I should definitively make this supported by plann" | | +| Resource-aware planning | 8-16 h | `--add-resource` exists; nothing plans around resource availability | | +| Scheduling: invitations and replies (RFC 6638 / iTIP) | 24-40 h | `TASK_MANAGEMENT.md`: "plann 1.0 does not support scheduling" | | +| A TUI | 40-60 h | `DESIGN.md`: "Perhaps a new calendar-tui for increased interactivity?" | | +| Issue-tracker backends (Gitea, GitLab, GitHub) | 12-20 h each | Cheap only *after* Phase 3; the abstraction is `calendaring-client`'s job | | +| Alarms and push notifications | 16-24 h | `TASK_MANAGEMENT.md` is ambivalent: "the very nature of plann is to deliver information on demand - pull, not push" | | +| Completing the calendaring-client migration | 16-24 h | Phase 3 buys a partial port; this buys the rest | | + +--- + +## References + +### Standards +- [RFC 5545 - iCalendar](https://datatracker.ietf.org/doc/html/rfc5545) +- [RFC 4791 - CalDAV](https://datatracker.ietf.org/doc/html/rfc4791) +- [RFC 6638 - CalDAV Scheduling](https://datatracker.ietf.org/doc/html/rfc6638) +- [RFC 7986 - New iCalendar Properties](https://datatracker.ietf.org/doc/html/rfc7986) +- [RFC 9073 - Event Publishing Extensions](https://datatracker.ietf.org/doc/html/rfc9073) +- [RFC 9074 - VALARM Extensions](https://datatracker.ietf.org/doc/html/rfc9074) +- [RFC 9253 - Support for iCalendar Relationships](https://datatracker.ietf.org/doc/html/rfc9253) + +### Related projects +- [python-caldav](https://github.com/python-caldav/caldav) - same author; plann's backend +- [icalendar-searcher](https://pypi.org/project/icalendar-searcher/) - same author; client-side filtering +- `calendaring-client` - same author, separately NLnet-funded; the Phase 3 target +- [calendar-cli](https://github.com/pycalendar/calendar-cli/) - plann's predecessor +- [khal](https://github.com/geier/khal) - the closest alternative, with an offline-first design + +### Internal documents +- [`DESIGN.md`](../DESIGN.md) - the pre-implementation design notes +- [`NEXT_LEVEL.md`](../NEXT_LEVEL.md) - the time-tracking and project-management vision +- [`TASK_MANAGEMENT.md`](../TASK_MANAGEMENT.md) - the task-management method +- [`USER_GUIDE.md`](../USER_GUIDE.md) - the (stale) user guide +- [`docs/code-review-2026-06-12.md`](code-review-2026-06-12.md) - 15 correctness bugs, fix status tracked +- [`docs/CODE_REVIEW.md`](CODE_REVIEW.md) - the earlier 2025 review +- [`docs/panic_algorithm.md`](panic_algorithm.md) - the panic-planning notes +- [`docs/CALENDAR_SERVER_RECOMMENDATIONS.md`](CALENDAR_SERVER_RECOMMENDATIONS.md) - which servers work diff --git a/docs/code-review-2026-06-12.md b/docs/code-review-2026-06-12.md new file mode 100644 index 0000000..69979d8 --- /dev/null +++ b/docs/code-review-2026-06-12.md @@ -0,0 +1,414 @@ +# Code review — plann + +This code review was started with the Fable model, but the model [got yanked](https://www.anthropic.com/news/fable-mythos-access) in the middle of the process, so the rest was done with Opus. That's sad, the whole point of asking for a code review was to utilize the Fable model. + +- **Date:** 2026-06-13 +- **Branch:** `refactor/delegate-config-to-caldav` +- **Commit:** `3e746c4` +- **Scope:** Full review of the `plann/` package (`lib.py`, `cli.py`, `commands.py`, + `interactive.py`, `panic_planning.py`, `timespec.py`, `config.py`, `template.py`). +- **Method:** 7 independent finder passes (line-by-line, removed-behavior, + cross-file, reuse, simplification, efficiency, altitude), then each correctness + candidate re-verified by reading the cited code. + +Findings are grouped: confirmed correctness bugs first (ranked most-severe), then +cross-cutting cleanup / altitude / efficiency themes. + +--- + +## Correctness bugs + +### 1. `add-time-tracking` subcommand crashes on every invocation — `cli.py:192` + +```python +@click.option('startnow/track', help="...", default=True) +``` + +The option name is missing its leading dashes (`--startnow/--track`). Click rejects +this with `ValueError: Invalid start character for option (startnow)` as soon as the +command parser is built, so `plann select ... add-time-tracking` — even +`add-time-tracking --help` — exits with an error and the feature is completely +unreachable. + +**Fix:** `@click.option('--startnow/--track', ...)`. + +**Human comment:** This feature was added in my working directory long ago, heavily used, but only through the interactive menus. + +### 2. `select --no-pinned-tasks --todo` raises `NameError`/`AttributeError` — `commands.py:160-162` + +```python +if isinstance(obj, caldav.Todo) and not pinned_tasks: + _relships_by_type(obj, 'CHILD').get('CHILD',[]) # result discarded + if not any(x.icalendar_comp.get('STATUS', '')!='CANCELLED' for x in parents if isinstance(x, caldav.Event)): +``` + +Two bugs in two lines: +- Line 161 computes the child relations but throws the result away. +- Line 162 iterates `parents`, which is only ever bound in the *Event* branch + (line 153). For the first `Todo` in the loop `parents` is undefined → `NameError`; + if an Event was processed earlier the **stale** `parents` from that Event is used. +- `x.icalendar_comp` is a typo for `x.icalendar_component` → `AttributeError`. + +The intent was presumably `children = _relships_by_type(obj, 'CHILD').get('CHILD',[])` +followed by a check over `children`. + +### 3. `_relationship_text` only ever shows the first relation type — `lib.py:377` + +```python +for reltype in rels: + objs = [] + for relobj in rels[reltype]: + objs.append(_summary(relobj)) + ret.append(reltype + "\n" + "\n".join(objs) + "\n") + return "\n".join(ret) # <-- indented inside the loop +``` + +The `return` sits inside the `for` loop, so the function returns after the first +`reltype`. A task with both PARENT and CHILD relations silently shows only one of +them. This text is displayed in `interactive_split_task` right before the user +decides how to split/postpone, so they act on an incomplete picture. + +**Fix:** dedent the `return` to function level. + +### 4. `_list(..., ics=True)` emits the first object regardless of the filter — `lib.py:441` + +```python +if ics: + if not objs: + return + icalendar = objs.pop(0).icalendar_instance # included unconditionally + for obj in objs: + if not filter(obj): + continue + icalendar.subcomponents.extend(obj.icalendar_instance.subcomponents) +``` + +The first object is popped and used as the base instance without ever calling +`filter(obj)`. If the filter would reject `objs[0]` (e.g. an interactive selection +that excludes completed tasks and the first task happens to be completed), it is +exported anyway. + +**Fix:** apply `filter` to the first object too — build the base calendar from the +first *accepted* object, or start from an empty calendar and extend for every +accepted object. + +### 5. Inverted condition in `interactive_split_task` — never postpones when asked — `interactive.py:336` + +```python +postpone = click.prompt("Should we postpone the parent task?", default='0h') +if postpone in ('0h', '0'): + _procrastinate([obj], postpone, check_dependent='interactive', ...) +``` + +The procrastinate call fires **only** when the user declines (enters `0h`/`0`) and +is skipped when they enter a real duration like `2d`. The logic is backwards — +compare the correct `not in` test used in `commands.py`. Result: answering `2d` +silently leaves the parent's DUE/DTSTART unchanged; answering the `0h` default does +a pointless zero-postpone. + +**Fix:** `if postpone not in ('0h', '0'):`. + +### 6. Year duration is ~15 days when there is no base date — `timespec.py:151` + +```python +time_units = { + 's': 1, 'm': 60, 'h': 3600, + 'd': 86400, 'w': 604800, + 'y': 1314000 # == 365*3600, missing the *24 day factor +} +``` + +`1314000 = 365 * 3600`; the correct value is `365 * 86400 = 31_536_000`. This table +is only consulted in the `dt is None` branch (the dated branch is special-cased at +line 163), so `parse_add_dur(None, '1y')` returns `timedelta(seconds=1_314_000)` +≈ 15.2 days — a 24× error. + +**Human comment:** Is it needed to reinvent the wheel here? Doesn't there exist good libraries out there doing the job? + +### 7. `parse_add_dur` year branch crashes on a `date` and on Feb 29 — `timespec.py:163-164` + +```python +if u=='y' and dt: + dt = datetime.datetime.combine(datetime.date(dt.year+int(i), dt.month, dt.day), dt.time(), tzinfo=dt.tzinfo) +``` + +`dt.time()` and `dt.tzinfo` don't exist on a plain `datetime.date`. The guard at +line 146 (`not isinstance(dt, datetime.date)`) treats both `date` and `datetime` as +"already parsed", so a `date` flows straight here. `parse_timespec('2021-01-08+1y')` +where `parse_dt` returned a `date` → `AttributeError`. Separately, a Feb-29 start +date raises `ValueError` in a non-leap target year. + +### 8. Multi-unit duration loses all but the last unit when there is no base date — `timespec.py:166` + +```python +else: + diff = datetime.timedelta(0, i*time_units[u]) # reassigned, never accumulated + if dt: + dt = dt + diff +... +return diff +``` + +When `dt is None`, each loop iteration overwrites `diff` instead of accumulating, and +the function returns only the **last** component. `parse_add_dur(None, '1h30m')` +returns 30 minutes, not 90. Reachable via `interactive.py:332` +`parse_add_dur(None, new_estimate)` where the user types e.g. `1h30m`. + +**Fix:** accumulate (`total += diff`) across iterations and return the sum. + +### 9. Event-only guard in `add-time-tracking` never fires — `cli.py:202` + +```python +if not startnow and not all (x for x in objs if isinstance(x, caldav.calendarobjectresource.Event)): + _abort("original timespan is only allowed for events ...") +``` + +`all(x for x in objs if isinstance(x, Event))` tests the truthiness of the Event +objects that *survive* the filter, not "are all objects Events". For an all-Todo +selection the generator is empty → `all([])` is `True` → the guard is skipped and +`add_time_tracking_` is called on a Todo with `start_time=None`. + +**Fix:** `all(isinstance(x, caldav.Event) for x in objs)`. (Blocked behind bug #1 +today, but wrong regardless.) + +### 10. `timeline_suggestion` crashes on all-day / DTEND-less events that have relations — `panic_planning.py:112` + +```python +if 'RELATED-TO' in comp and event.get_dtend()>_now(): +``` + +`get_dtend()` can return `None` (no DTEND) or a `date` (all-day event); neither can +be compared with the aware `datetime` from `_now()`. An all-day event carrying a +`RELATED-TO` property raises `TypeError: can't compare datetime.datetime to +datetime.date`. This line is **outside** the `try/except AssertionError` that guards +`timeline.add_event`, so the whole panic check aborts. + +**Fix:** guard for `None` and normalize date→datetime (e.g. via `_ensure_ts`) before +comparing. + +### 11. `interactive check-due --limit` is a string, not an int — `cli.py:519` + +```python +@click.option('--limit', help='If more than limit overdue tasks ...') # no type=int +``` + +`select`'s `--limit` declares `type=int` (`cli.py:169`), but this one doesn't. The +string flows into `__select`, which does `ctx.obj['objs'][0:limit]` +(`commands.py:191`) → `TypeError: slice indices must be integers`. +`plann interactive check-due --limit 5` crashes. + +**Fix:** add `type=int`. + +### 12. `obj.icalendar_component_UID` in the inconsistency-logging path — `lib.py:355` + +```python +logging.error(f"Inconsistency issue ... (UID={obj.icalendar_component_UID}, ...)") +``` + +There is no `icalendar_component_UID` attribute (line 351 correctly uses +`obj.icalendar_component['UID']`). When a relative has more than one RELATED-TO +pointing back (`len(back_rel_types) > 1`), the log statement itself raises +`AttributeError` instead of logging — turning a recoverable data warning into a +crash during e.g. `list --top-down`. + +### 13. Leftover `breakpoint()` in `_procrastinate` — `lib.py:241-244` + +```python +import inspect +stack_depth = len(inspect.stack()) +if stack_depth > 13: + breakpoint() +``` + +Debugging scaffolding left in. Interactive procrastination of a task with a +postponable parent is easily reached through nested click subcommands with a stack +deeper than 13 frames, dropping the user into `pdb` (or appearing to hang in +non-tty/scripted use). The existing `assert recursivity < 16` already guards runaway +recursion. **Delete the block.** + +### 14. `time_tracking` config given as a string iterates per character — `lib.py:165` + +```python +time_tracking = getattr(obj.parent, 'extra_config', {}).get('time_tracking') +... +for tt in time_tracking: + if tt in ('timewarrior', 'Timewarrior', 'timew'): + ... + else: + raise NotImplementedError('Only time tracking through taskw supported so far') +``` + +The error message at line 158 literally tells the user to set +`time_tracking=timewarrior` — a scalar. A scalar string then gets iterated +character-by-character (`'t'`, `'i'`, ...), none match, and the `else` raises. A +plausibly-correct config crashes. **Fix:** accept a string (wrap in a list) or +document/validate that a list is required. + +### 15. `dismiss-panic` double-prefixes its lookahead — `cli.py:538` + `commands.py:466` + +`dismiss_panic` (CLI) passes `f"+{lookahead}"` into `_dismiss_panic`, which *also* +does `lookahead = f"+{lookahead}"` at `commands.py:466`, yielding `"++60d"`. The +direct callers at `cli.py:508/510` pass no prefix, so only the CLI command path is +affected. It currently survives only because the duration regex tolerates the extra +`+`; passing `--lookahead='+60d'` would push it to `"+++60d"`. **Fix:** prefix in +exactly one place. + +--- + +## Cleanup / altitude / efficiency + +These are not crashes but raise maintenance cost or risk; several were flagged +independently by multiple finder passes. + +### Duplication & drift + +- **`commands.py:226` — `_interactive_edit` is a stale copy of `interactive.py:208`.** + `commands.py` already imports many helpers from `plann.interactive`, but keeps its + own copy of `_interactive_edit`. The two have *already* diverged: the + `interactive.py` version has the just-ported `start` time-tracking command and + re-prompts after it; the `commands.py` copy (used by `_check_due` and + `_dismiss_panic`) does not. The new `start` feature is effectively unreachable + through those paths. **Delete the copy, import the canonical one.** +- **`commands.py:282` — pdb hand-off block duplicates `interactive.py:88-94`.** Same + "happy hacking" text + `breakpoint()`. Extract a shared `_pdb_edit(obj)`. +- **`interactive.py:301` — inline summary fallback duplicates `_summary`.** + `comp.get('summary') or comp.get('description') or comp.get('uid')` re-implements + the already-imported `_summary`; the two will drift. +- **`interactive.py:153` — `get_obj` helper duplicates `_get_obj_from_line` + (`interactive.py:366`).** Two line→object parsers with different edge-case + handling (comment stripping, empty-UID behavior). + +### Duration grammar in four places + +The `[smhdwy]` relative-duration grammar is encoded separately in +`commands.py:115` (`__select`), `timespec.py:154` (`parse_add_dur`), +`timespec.py:209` (`_parse_timespec`), and `interactive.py:381` +(`_command_line_edit`). Adding the planned month unit (TODO in `parse_add_dur`) +means `--end=+3M` parses in one place and is rejected in another. Export a single +`DURATION_RE` / `is_duration()` from `timespec.py`. + +### Component-type detection by raw-string sniffing + +`'BEGIN:VTODO' in obj.data` / `'BEGIN:VEVENT' in obj.data` appears across +`commands.py:227`, `panic_planning.py:106/116/121`, `cli.py:404`, +`interactive.py:209`. A single `lib.py` helper using `obj.icalendar_component.name` +(already used at `lib.py:140`) would centralize it; otherwise each new component +type (the VJOURNAL work already touched several of these) means hunting down 7+ +scattered checks, and any object whose *description* text contains `BEGIN:VEVENT` +misclassifies. + +### `category` vs `categories` special-casing + +The singular/plural oddball is special-cased at `lib.py:41`, `lib.py:392-399` +(`_process_set_arg`), `lib.py:418` (`_set_something`), `cli.py:139`, and +`commands.py:558`. Adding another multi-valued attribute (attendee, etc.) needs +synchronized edits in five places across three modules. + +### Config-to-caldav migration left half-done + +`config.py:45` (`interactive_config`) still hardcodes its own key list +(`caldav_url`, `ssl_verify_cert`, ...) although connecting/parsing was delegated to +`caldav.config` in commit `26dc550`. The writer half already diverges from what +`find_calendars` understands (no `features`, no `time_tracking`/`extra_config` from +commit `45c5c22`). Keys the prompt writes that caldav's extractor drops are silently +ignored at connect time with no error. + +### `extra_config` smuggled as a monkey-patched attribute + +`find_calendars` (`lib.py:110`) attaches `cal.extra_config` onto caldav Calendar +objects, and `add_time_tracking` (`lib.py:156`) reads it via +`getattr(obj.parent, 'extra_config', {})`. Any calendar object not produced by +`find_calendars` (e.g. `obj.parent` after an `object_by_uid` round-trip) silently +lacks it, so time tracking raises `NotImplementedError` despite correct config. This +per-calendar config should ride through the caldav config/calendar API rather than an +injected attribute. + +### Hand-rolled implementations of stdlib / library functions + +- **`interactive.py:344` (`_editor`)** re-implements a PATH search that + `shutil.which(editor)` does in one line — and the hand-rolled version leaves `ed` + bound to a non-executable fallback when nothing is found, producing a confusing + `FileNotFoundError`. +- **`lib.py:72` (`_split_vcals`)** splits concatenated VCALENDAR streams by raw + string scanning at a hard-coded 14-char offset, assuming LF line endings; + `icalendar.Calendar.from_ical(ical, multiple=True)` (icalendar 5.0.7 is pinned) + handles CRLF and folding correctly. + +### Efficiency + +- **`cli.py:94`** — the `cli()` group calls `find_calendars()` (network discovery + + per-calendar connect) unconditionally, so even `plann --help` connects to every + configured calendar. Defer discovery to first use. +- **`commands.py:177`** — the sort key rebuilds `Template(skey)` on every comparison; + compile it once outside `fkey`. +- **`lib.py:348`** — the relationship consistency check does a `get_relatives` + network round-trip per related object, and `_list` calls `_relships_by_type` per + listed object, so `list --top-down` over N tasks with R relations issues ~N×R + extra round-trips. Cache fetched relatives or make the scan opt-in. +- **`commands.py:562`** — `_set_task_attribs` issues a fresh server `_select` per + attribute (category, due, priority, duration) plus another in `_cats`; fetch once + and filter client-side. +- **`commands.py:92`** — `--uid` resolution loops `get_object_by_uid` per + (uid × calendar) and keeps querying after a hit: U×C round-trips for U uids over C + calendars. + +### Dead code + +- **`timespec.py:229`** — `raise NotImplementedError("possibly a ISO time interval")` + is unreachable; every preceding path returns or raises. +- **`interactive.py:58-64`** — `command_edit` checks `if 'with family' in command` + twice in the same block; the second was probably meant to be `'with parent'`, so + `postpone 1d with parent` silently does nothing. + +--- + +## Suggested priority + +Fix in this order: **#1** (feature dead on arrival), **#2** (crashes a common +listing command), **#5 / #3 / #4** (silent wrong behavior — the dangerous kind), +then the remaining crash-on-edge-case items **#6-#15**. The duplication of +`_interactive_edit` is worth doing early since it currently hides the newly-ported +`start` feature from two command paths. + +> Note: filename uses `2026-06-12` per request; the review was actually run +> 2026-06-13. + +--- + +## Fix status (updated 2026-06-13) + +| # | Title | Status | Notes | +|---|-------|--------|-------| +| 1 | `add-time-tracking` crashes — missing `--` on option | ✅ Fixed | `cli.py:192` | +| 2 | `--no-pinned-tasks --todo` raises `NameError`/`AttributeError` | ✅ Fixed | `commands.py:160-162` | +| 3 | `_relationship_text` only shows first relation type | ✅ Fixed | `lib.py:377` — `return` dedented | +| 4 | `_list(..., ics=True)` skips filter on first object | ✅ Fixed | `lib.py:441` | +| 5 | Inverted condition in `interactive_split_task` — never postpones | ✅ Fixed | `interactive.py:336` | +| 6 | Year duration ~15 days (missing `*24`) | ✅ Fixed | `timespec.py` — corrected constant; `y` branch now uses `relativedelta` | +| 7 | `parse_add_dur` year branch crashes on `date` / Feb 29 | ✅ Fixed | `timespec.py` — `relativedelta(years=n)` handles both | +| 8 | Multi-unit duration loses all but last unit (no `dt`) | ✅ Fixed | `timespec.py` — accumulate `diff` instead of overwriting | +| 9 | Event-only guard in `add-time-tracking` never fires | ✅ Fixed | `cli.py:202` — fixed `all()` | +| 10 | `timeline_suggestion` crashes on all-day / DTEND-less events | ✅ Fixed | `panic_planning.py:112` | +| 11 | `interactive check-due --limit` is a string, not int | ✅ Fixed | `cli.py:519` — added `type=int` | +| 12 | `obj.icalendar_component_UID` crashes in inconsistency-log path | ✅ Fixed | `lib.py:355` | +| 13 | Leftover `breakpoint()` in `_procrastinate` | ✅ Fixed | `lib.py:241-244` | +| 14 | `time_tracking` string iterated char-by-char | ✅ Fixed | `lib.py:165` — wrap scalar in list | +| 15 | `dismiss-panic` double-prefixes lookahead (`++60d`) | ✅ Fixed | `cli.py:538` + `commands.py:466` | +| C1 | `_interactive_edit` duplicated in `commands.py` (stale copy) | ✅ Fixed | Deleted copy, import canonical from `interactive` | +| C2 | `pdb` hand-off block duplicated | ✅ Fixed | Extracted `_pdb_edit(obj)` in `interactive.py` | +| C3 | Inline summary fallback duplicates `_summary` | ✅ Fixed | `interactive.py:301` uses `_summary(obj)` | +| C4 | `get_obj` / `_get_obj_from_line` duplicate parsers | ✅ Fixed | Deleted `get_obj` closure, use `_get_obj_from_line` | +| C5 | Duration grammar encoded in 4 places | ✅ Fixed | `timespec.py` exports `DURATION_UNITS`/`DURATION_RE`/`DURATION_TOKEN_RE`/`is_duration`; all 4 sites use them | +| C6 | Component-type detection by raw-string sniffing | ✅ Fixed | `lib.py` exports `_component_type`/`_caldav_objclass`; all object/raw sites use them | +| C7 | `category` vs `categories` special-cased in 5 places | ✅ Fixed | `lib.COMMA_LIST_ATTRS` registry + helpers; edit path centralised; also generalised to `resources` and added `--add-categories`/`--add-resource`/`--add-resources`; `--set-category` deprecated. Select-by `--category`(substring)/`--categories`(exact) left intact (caldav search semantics) | +| C8 | `interactive_config` key list hardcoded / diverged | ✅ Fixed | Was also dead code (orphaned since the argparse→click migration). Re-wired as `plann configure`; connection prompt keys derived from `caldav.config.CONNKEYS` (asserts no drift); fixed `ssl_verify_cert`→`caldav_ssl_verify_cert`, added `features`/`calendar_name`/`extra_config.time_tracking`, dropped never-read `language`/`timezone` | +| C9 | `extra_config` smuggled as monkey-patched attribute | ❌ TODO | Cleanup | +| C10 | `_editor` re-implements `shutil.which` | ✅ Fixed | `interactive.py` — uses `shutil.which`, raises clear error if no editor found | +| C11 | `_split_vcals` hand-rolls VCALENDAR parsing | ✅ Fixed | Now `icalendar.Calendar.from_ical(ical, multiple=True)`; handles CRLF (the LF-only scanner returned nothing on CRLF input). Tests added | +| E1 | `find_calendars()` called unconditionally (even `--help`) | ✅ Fixed | Discovery deferred via `_LazyCalendars` wrapper; `plann --help` no longer connects | +| E2 | `Template(skey)` rebuilt on every sort comparison | ✅ Fixed | Sort-key logic extracted to `_sort_key_function`; template compiled once per key. Tests added | +| E3 | `_relships_by_type` N×R round-trips in `list --top-down` | ✅ Fixed | Per-traversal `_RelativeCache` memoizes object-by-UID fetches and the relationship scan; `_relships_by_type` now parses related UIDs locally (`fetch_objects=False`) and resolves them through the cache, threaded through the `_list` recursion. (Caching, not opt-in - relationships are still always scanned for the hierarchical view.) Test added | +| E4 | `_set_task_attribs` issues a fresh server `_select` per attribute | ✅ Fixed | Fetches the pending todos once, then filters client-side per attribute via `icalendar_searcher` (`undef` operator - same semantics as the server-side `no_` search); the category branch reuses the same fetch instead of re-querying. 5 round-trips → 1. Tests added | +| E5 | `--uid` resolution keeps querying after a hit | 🚫 Won't fix | By design: the same UID may exist in several calendars (copied event/task) and `--uid` collects all matches. Breaking on first hit would silently drop cross-calendar duplicates; the U×C lookups are inherent to "find this UID in any calendar" (no batch-by-UID query exists). Documented in code | +| D1 | `raise NotImplementedError` unreachable at `timespec.py:229` | ✅ Fixed | Removed unreachable statement | +| D2 | `'with family'` checked twice in `command_edit` | ✅ Fixed | `interactive.py` — second check now `'with parent'` → `with_parent` | diff --git a/docs/panic_algorithm.md b/docs/panic_algorithm.md new file mode 100644 index 0000000..3ab6cb7 --- /dev/null +++ b/docs/panic_algorithm.md @@ -0,0 +1,18 @@ +This was internal meeting notes with my rubber duck long ago, when doing the panic planning algorithm. It's been in the repository as an untracked file for ages. + +* Overdue has to be dealt with before. Start with the lowest priority (highest number) and offer to procrastinate all overdue, all near-due and all others + +* the panic algorithm should have an end time, default "in 30 days"? +* we should have some hours_per_day, max number of hours we can work per day. Defaults to 4? Max 24. +* load all events for the period, sorted by dtstart +* create "mock" events with "free time" and inject into the same list +* for all priorities 1 ... 9 ... +* for all (selected) tasks with given priority, reverse sorted by due ... +* go through available time slots reversively, starting from the due and ending up at now +* calculate the required slack time (`task_duration*(24-hours_per_day)/241), add it to slack balance +* for a free time slot not big enough to accomodate the full task ... +* if we have slack balance, use the time slot for slack, otherwise ignore it. +* for a free time slot big enough to accomodate the full task ... +* if `slot_time` > `task_duration` + `slack_balance`, then inject the slack time balance at the end of the time slot and task right before it. +* else, put task at the start of the time slot, the rest of the time slot is slack, and we get a negative slack balance +* if we're at the head of the period ("now"), then ... time to panic! Offer to procrastinate all tasks with same or higher priority diff --git a/.lychee.toml b/lychee.toml similarity index 100% rename from .lychee.toml rename to lychee.toml diff --git a/plann/cli.py b/plann/cli.py index f4e1324..5251012 100755 --- a/plann/cli.py +++ b/plann/cli.py @@ -38,9 +38,21 @@ _split_high_pri_tasks, _split_huge_tasks, ) -from plann.config import config_section, expand_config_section, read_config +from plann.config import config_section, expand_config_section, interactive_config, read_config from plann.interactive import _abort -from plann.lib import _list, _split_vcal, _split_vcals, attr_int, attr_time, attr_txt_many, attr_txt_one, find_calendars +from plann.lib import ( + COMMA_LIST_ATTRS, + _caldav_objclass, + _list, + _split_vcal, + _split_vcals, + _summary, + attr_int, + attr_time, + attr_txt_many, + attr_txt_one, + find_calendars, +) from plann.lib import add_time_tracking as add_time_tracking_ from plann.timespec import _now, parse_dt, tz @@ -60,6 +72,40 @@ ## See https://click.palletsprojects.com/en/8.0.x/api/#click.ParamType and ## /usr/lib/*/site-packages/click/types.py on how to do this. +class _LazyCalendars: + """A list-like wrapper that defers calendar discovery until first use. + + Discovery hits the network (one connect per configured calendar), so we + only want to pay for it when a command actually uses the calendars - not + when click merely runs the group callback to e.g. show subcommand help + (code review E1). Supports the handful of operations the commands use: + iteration, ``len()``, indexing and truthiness. + """ + def __init__(self, discover): + self._discover = discover + self._resolved = None + + @property + def _calendars(self): + if self._resolved is None: + self._resolved = self._discover() + return self._resolved + + def __iter__(self): + return iter(self._calendars) + + def __len__(self): + return len(self._calendars) + + def __getitem__(self, index): + return self._calendars[index] + + def __bool__(self): + return bool(self._calendars) + + def extend(self, more): + self._calendars.extend(more) + @click.group() @click.version_option(None, "--version", "-V", package_name="plann") ## TODO: interactive config building @@ -89,17 +135,29 @@ def cli(ctx, **kwargs): ctx.ensure_object(dict) ## TODO: add all relevant connection parameters for the DAVClient as options ## TODO: logic to read the config file and edit kwargs from config file - ## TODO: delayed communication with caldav server (i.e. if --help is given to subcommand) ## TODO: catch errors, present nice error messages - ctx.obj['calendars'] = find_calendars(kwargs, kwargs['raise_errors']) + ## Calendar discovery talks to the network; defer it (E1) so commands that + ## never touch the server - notably `plann --help` - don't + ## connect to every configured calendar. + ctx.obj['calendars'] = _LazyCalendars(lambda: _discover_calendars(kwargs)) + ## stashed for the `configure` subcommand (and any other command that needs + ## to know which config file / section the user pointed at) + ctx.obj['config_file'] = kwargs['config_file'] + ctx.obj['config_section'] = kwargs['config_section'] for flag in ('show_native_timezone', 'store_timezone', 'implicit_timezone'): setattr(tz, flag, kwargs[flag]) + +def _discover_calendars(kwargs): + """Find calendars from the command-line connection options and, unless + --skip-config is given, from every selected config-file section.""" + calendars = find_calendars(kwargs, kwargs['raise_errors']) if not kwargs['skip_config']: config = read_config(kwargs['config_file']) if config: for meta_section in kwargs['config_section']: for section in expand_config_section(config, meta_section): - ctx.obj['calendars'].extend(find_calendars(config_section(config, section), raise_errors=kwargs['raise_errors'])) + calendars.extend(find_calendars(config_section(config, section), raise_errors=kwargs['raise_errors'])) + return calendars @cli.command() @click.pass_context @@ -116,6 +174,20 @@ def list_calendars(ctx): lines = [f"{name:<{max_display_name}} {url}" for name, url in calendar_info] click.echo_via_pager(output + "\n".join(lines) + "\n") +@cli.command() +@click.pass_context +def configure(ctx): + """ + Interactive configuration mode (EXPERIMENTAL - under-tested, here be dragons). + + Prompts for connection parameters and writes them to the config file. + """ + config_file = ctx.obj['config_file'] + sections = ctx.obj['config_section'] + section = sections[0] if sections else 'default' + config = read_config(config_file, interactive_error=True) or {} + interactive_config(config, config_file, config_section=section) + def _set_attr_options_(func, verb, desc=""): """ decorator that will add options --set-category, --set-description etc @@ -129,6 +201,12 @@ def _set_attr_options_(func, verb, desc=""): if verb == 'no-': for foo in attr_txt_one + attr_txt_many + attr_time + attr_int: func = click.option(f"--{verb}{foo}/--has-{foo}", default=None, help=f"{desc} ical attribute {foo}")(func) + elif verb == 'add-': + ## append-options, only for the comma-list properties, in both the + ## plural (comma-split) and singular (comma-literal) form + for plural, singular in COMMA_LIST_ATTRS.items(): + for foo in (singular, plural): + func = click.option(f"--{verb}{foo}", help=f"{desc} ical attribute {plural}", multiple=True)(func) else: if verb == 'set-': attr__one = attr_txt_one + attr_time + attr_int @@ -136,8 +214,18 @@ def _set_attr_options_(func, verb, desc=""): attr__one = attr_txt_one for foo in attr__one: func = click.option(f"--{verb}{foo}", help=f"{desc} ical attribute {foo}")(func) - for foo in attr_txt_many + ['categories']: ## TODO: category is the oddball, not categories - func = click.option(f"--{verb}{foo}", help=f"{desc} ical attribute {foo}", multiple=True)(func) + ## the extra 'categories' gives the plural form for the comma-list + ## attributes (attr_txt_many carries the singular 'category' as that is + ## the substring-search form; see lib.COMMA_LIST_ATTRS) + singular_to_plural = {s: p for p, s in COMMA_LIST_ATTRS.items()} + for foo in attr_txt_many + ['categories']: + help_text = f"{desc} ical attribute {foo}" + if verb == 'set-' and foo in singular_to_plural: + ## e.g. --set-category: kept for backwards compatibility, but it + ## *appends* rather than replaces, which the name does not convey + help_text = (f"{desc} ical attribute {foo} (DEPRECATED - this appends; use " + f"--add-{foo} to append or --set-{singular_to_plural[foo]} to replace)") + func = click.option(f"--{verb}{foo}", help=help_text, multiple=True)(func) return func def _set_attr_options(verb="", desc=""): @@ -148,7 +236,8 @@ def _set_attr_options(verb="", desc=""): @click.option('--mass-interactive/--no-mass-interactive-select', help="editor based interactive filtering") @click.option('--all/--none', default=None, help='Select all (or none) of the objects. Overrides all other selection options.') @click.option('--uid', multiple=True, help='select an object with a given uid (or select more object with given uids). Overrides all other selection options') -@click.option('--abort-on-missing-uid/--ignore-missing-uid', default=False, help='Abort if (one or more) uids are not found (default: silently ignore missing uids). Only effective when used with --uid') +@click.option('--abort-on-missing-uid/--ignore-missing-uid', default=False, help='Abort if (one or more) uids are not found (default: carry on with whatever was found). Only effective when used with --uid') +@click.option('--warn-on-missing-uid/--no-warn-on-missing-uid', default=True, help='Print a warning (to stderr) for uids that are not found (default: warn). Has no effect when --abort-on-missing-uid is given. Only effective when used with --uid') @click.option('--todo/--no-todo', default=None, help='select only todos (or no todos)') @click.option('--event/--no-event', default=None, help='select only events (or no events)') @click.option('--journal/--no-journal', default=None, help='select only journal entries (or no journal entries)') @@ -189,7 +278,7 @@ def select(*largs, **kwargs): @select.command() @click.pass_context -@click.option('startnow/track', help="the event starts now vs track the original timespan", default=True) +@click.option('--startnow/--track', help="the event starts now vs track the original timespan", default=True) def add_time_tracking(ctx, startnow): """ Track time spent on events/tasks @@ -199,7 +288,7 @@ def add_time_tracking(ctx, startnow): start_time = _now() else: start_time = None - if not startnow and not all (x for x in objs if isinstance(x, caldav.calendarobjectresource.Event)): + if not startnow and not all(isinstance(x, caldav.Event) for x in objs): _abort("original timespan is only allowed for events - and you've selected tasks or journals") if len(objs)>1 and startnow: _abort("Only one event/task can be started at the time") @@ -224,14 +313,15 @@ def list_categories(ctx): @select.command() @click.option('--ics/--no-ics', default=False, help="Output in ics format") @click.option('--template', default="{DTSTART:?{DUE:?(date missing)?}?%F %H:%M:%S %Z}: {SUMMARY:?{DESCRIPTION:?(no summary given)?}?}") +@click.option('--separator', default="\n", help="String to separate the listed items with (defaults to a newline)") @click.option('--top-down/--flat-list', help="Check relations and list the relations in a hierarchical way") @click.option('--bottom-up/--flat-list', help="List parents (dependencies) in a hierarchical way (cannot be combined with top-down)") @click.pass_context -def list(ctx, ics, template, top_down=False, bottom_up=False): +def list(ctx, ics, template, separator="\n", top_down=False, bottom_up=False): """ Print out a list of tasks/events/journals """ - return _list(ctx.obj['objs'], ics, template, top_down=top_down, bottom_up=bottom_up) + return _list(ctx.obj['objs'], ics, template, top_down=top_down, bottom_up=bottom_up, separator=separator) @select.command() @@ -261,17 +351,20 @@ def delete(ctx, multi_delete, **kwargs): Delete the selected item(s) """ objs = ctx.obj['objs'] + if not objs: + click.echo("No items selected for deletion") + return if multi_delete is None and len(objs)>1: multi_delete = click.confirm(f"OK to delete {len(objs)} items?") if len(objs)>1 and not multi_delete: _abort(f"Not going to delete {len(objs)} items") for obj in objs: + click.echo(f"Deleting {_summary(obj)}") obj.delete() ## TODO: reconsider the naming of the attributes and functions - --mass-interactive should probably be --interactive-editor - and the interactive reprioritization function needs to be renamed @select.command() @click.option('--pdb/--no-pdb', default=None, help="Interactive edit through pdb (experts only)") -@click.option('--add-category', default=None, help="Add a category (equivalent with --set-category, while --set-categories will overwrite existing categories))", multiple=True) @click.option('--postpone', help="Add something to the DTSTART and DTEND/DUE") @click.option('--postpone-with-children', help="Add something to the DTSTART and DTEND/DUE for this and children") @click.option('--interactive-ical/--no-interactive-ical', help="Edit the ical interactively") @@ -284,6 +377,7 @@ def delete(ctx, multi_delete, **kwargs): @click.option('--complete/--uncomplete', default=None, help="Mark task(s) as completed") @click.option('--complete-recurrence-mode', default='safe', help="Completion of recurrent tasks, mode to use - can be 'safe', 'thisandfuture' or '' (see caldav library for details)") @_set_attr_options(verb='set') +@_set_attr_options(verb='add', desc='append to') @click.pass_context def edit(*largs, **kwargs): """ @@ -400,7 +494,7 @@ def ical(ctx, ical_data, ical_file): for c in ctx.obj['calendars']: ## TODO: there is a TODO-comment in add_object that objclass should not be mandatory. ## when that one has been fixed, remove this additional logic - objclass = caldav.Todo if "BEGIN:VTODO" in ical else (caldav.Journal if "BEGIN:VJOURNAL" in ical else caldav.Event) + objclass = _caldav_objclass(ical) c.add_object(objclass, ical) @add.command() @@ -515,7 +609,7 @@ def manage_tasks(ctx): _agenda(ctx) @interactive.command() -@click.option('--limit', help='If more than limit overdue tasks are found, probably we should do a mass procrastination rather than going through one and one task') +@click.option('--limit', type=int, help='If more than limit overdue tasks are found, probably we should do a mass procrastination rather than going through one and one task') @click.option('--lookahead', help='Look-ahead time - check tasks that needs to be completed in the near future', default='+16h') @click.pass_context def check_due(ctx, limit, lookahead): @@ -534,7 +628,7 @@ def dismiss_panic(ctx, hours_per_day, lookahead='60d'): Search for panic points, checks if they can be solved by procrastinating tasks, comes up with suggestions """ - return _dismiss_panic(ctx, hours_per_day, f"+{lookahead}") + return _dismiss_panic(ctx, hours_per_day, lookahead) @interactive.command() diff --git a/plann/commands.py b/plann/commands.py index 7dbf51f..58364f6 100644 --- a/plann/commands.py +++ b/plann/commands.py @@ -7,21 +7,27 @@ ## TODO: can we remove the click-dependency? import click +from icalendar_searcher import Searcher from plann.interactive import ( _abort, _editor, _get_obj_from_line, + _interactive_edit, _interactive_ical_edit, _interactive_relation_edit, _mass_interactive_edit, _mass_reprioritize, + _pdb_edit, _strip_line, - command_edit, interactive_split_task, ) from plann.lib import ( - _add_category, + _add_comma_list, + _comma_list_canonical, + _comma_list_tokens, + _component_type, + _is_comma_list_attr, _list, _process_set_arg, _procrastinate, @@ -34,7 +40,7 @@ ) from plann.panic_planning import timeline_suggestion from plann.template import Template -from plann.timespec import _ensure_ts, _now, parse_add_dur, parse_dt, parse_timespec, tz +from plann.timespec import DURATION_RE, _ensure_ts, _now, parse_add_dur, parse_dt, parse_timespec, tz def _select(ctx, interactive=False, mass_interactive=False, **kwargs): @@ -62,7 +68,30 @@ def _select(ctx, interactive=False, mass_interactive=False, **kwargs): if click.confirm(f"select {_summary(obj)}?"): ctx.obj['objs'].append(obj) -def __select(ctx, extend_objects=False, all=None, uid=[], abort_on_missing_uid=None, sort_key=[], skip_parents=None, skip_children=None, limit=None, offset=None, freebusyhack=None, pinned_tasks=None, **kwargs_): +def _sort_key_function(skey): + """Build a ``(reverse, key_function)`` pair from a single sort-key spec. + + A leading ``-`` reverses the sort. A spec containing ``{`` is treated as + a template and compiled once here - not rebuilt on every comparison during + the sort (code review E2). ``get_duration()`` is special-cased; any other + spec is a plain icalendar component property name. + """ + reverse = skey.startswith('-') + if reverse: + skey = skey[1:] + if '{' in skey: + template = Template(skey) + def fkey(obj): + return template.format(**obj.icalendar_component) + elif skey == 'get_duration()': + def fkey(obj): + return obj.get_duration() + else: + def fkey(obj): + return obj.icalendar_component.get(skey) + return reverse, fkey + +def __select(ctx, extend_objects=False, all=None, uid=[], abort_on_missing_uid=None, warn_on_missing_uid=True, sort_key=[], skip_parents=None, skip_children=None, limit=None, offset=None, freebusyhack=None, pinned_tasks=None, **kwargs_): """ select/search/filter tasks/events, for listing/editing/deleting, etc """ @@ -88,6 +117,14 @@ def __select(ctx, extend_objects=False, all=None, uid=[], abort_on_missing_uid=N kwargs[kw] = kwargs_[kw] ## uid(s) + ## NB: we deliberately query every calendar and collect every match rather + ## than stopping at the first hit - the same UID may legitimately exist in + ## more than one calendar (e.g. a copied event/task), and the caller may + ## want to act on all of them. This costs one lookup per (uid, calendar), + ## but that is inherent to "find this UID in any of my calendars": to know + ## whether a UID is in a calendar you have to ask that calendar. Do not + ## "optimise" this into a break-on-first-match - it would silently drop the + ## cross-calendar duplicates. missing_uids = [] for uid_ in uid: cnt = 0 @@ -99,8 +136,11 @@ def __select(ctx, extend_objects=False, all=None, uid=[], abort_on_missing_uid=N pass if not cnt: missing_uids.append(uid_) - if abort_on_missing_uid and missing_uids: - _abort(f"Did not find the following uids in any calendars: {missing_uids}") + if missing_uids: + if abort_on_missing_uid: + _abort(f"Did not find the following uids in any calendars: {missing_uids}") + elif warn_on_missing_uid: + click.echo(f"Warning: did not find the following uids in any calendars: {missing_uids}", err=True) if uid: return @@ -112,7 +152,7 @@ def __select(ctx, extend_objects=False, all=None, uid=[], abort_on_missing_uid=N if kwargs_.get('start'): kwargs['start'] = parse_dt(kwargs['start']) if kwargs_.get('end') and not isinstance(kwargs_.get('end'), datetime.date): - rx = re.match(r'\+((\d+(\.\d+)?[smhdwy])+)', kwargs['end']) + rx = re.match(rf'\+{DURATION_RE.pattern}', kwargs['end']) if rx: kwargs['end'] = parse_add_dur(kwargs.get('start', datetime.datetime.now()), rx.group(1)) else: @@ -158,30 +198,15 @@ def __select(ctx, extend_objects=False, all=None, uid=[], abort_on_missing_uid=N else: ret_objs.append(obj) if isinstance(obj, caldav.Todo) and not pinned_tasks: - _relships_by_type(obj, 'CHILD').get('CHILD',[]) - if not any(x.icalendar_comp.get('STATUS', '')!='CANCELLED' for x in parents if isinstance(x, caldav.Event)): + children = _relships_by_type(obj, 'CHILD').get('CHILD',[]) + if not any(x.icalendar_component.get('STATUS', '')!='CANCELLED' for x in children if isinstance(x, caldav.Event)): ret_objs.append(obj) ctx.obj['objs'] = ret_objs ## OPTIMIZE TODO: sorting the list multiple times rather than once is a bit of brute force, if there are several sort keys and long list of objects, we should sort once and consider all sort keys while sorting ## TODO: Consider that an object may be expanded and contain lots of event instances. We will then need to expand the caldav.Event object into multiple objects, each containing one recurrance instance. This should probably be done on the caldav side of things. for skey in reversed(sort_key): - ## If the key starts with -, sorting should be reversed - if skey[0] == '-': - reverse = True - skey=skey[1:] - else: - reverse = False - ## if the key contains {}, it should be considered to be a template - if '{' in skey: - def fkey(obj): - return Template(skey).format(**obj.icalendar_component) - elif skey == 'get_duration()': - def fkey(obj): - return obj.get_duration() - else: - def fkey(obj): - return obj.icalendar_component.get(skey) + reverse, fkey = _sort_key_function(skey) ctx.obj['objs'].sort(key=fkey, reverse=reverse) ## OPTIMIZE TODO: this is also suboptimal, if ctx.obj is a very long list @@ -215,45 +240,44 @@ def fkey(obj): if attr == 'SUMMARY': comp[attr] = freebusyhack -def _cats(ctx): +def _categories_in_use(objs): categories = set() - for obj in ctx.obj['objs']: + for obj in objs: cats = obj.icalendar_component.get('categories') if cats: categories.update(cats.cats) return categories -def _interactive_edit(obj): - if 'BEGIN:VEVENT' in obj.data: - objtype = 'event' - elif 'BEGIN:VTODO' in obj.data: - objtype = 'todo' - elif 'BEGIN:VJOURNAL' in obj.data: - objtype = 'journal' - else: - assert False - if objtype != 'todo': - raise NotImplementedError("interactive editing only implemented for tasks") - comp = obj.icalendar_component - summary = _summary(comp) - dtstart = comp.get('DTSTART') - pri = comp.get('PRIORITY', 0) - due = obj.get_due() - if not dtstart or not due: - click.echo(f"task without dtstart or due found, please run set-task-attribs subcommand. Ignoring {summary}") - return - dtstart = _ensure_ts(dtstart) - click.echo(f"pri={pri} {dtstart:%F %H:%M:%S %Z} - {due:%F %H:%M:%S %Z}: {summary}") - input = click.prompt("postpone d / ignore / part(ially-complete) / complete / split / cancel / set foo=bar / edit / family / pdb?", default='ignore') - command_edit(obj, input, interactive=True) +def _cats(ctx): + return _categories_in_use(ctx.obj['objs']) + +def _todos_missing(objs, prop): + """Return the objects from ``objs`` that have no ``prop`` property set. + + Client-side filtering via icalendar_searcher's ``undef`` operator - the + same semantics the caldav library uses for the server-side ``no_`` + search, so we can fetch the task list once and filter locally rather than + issuing one server query per attribute (code review E4).""" + searcher = Searcher(todo=True) + searcher.add_property_filter(prop, None, operator="undef") + + def _missing(obj): + ## "undef" only catches a property that is absent altogether. An + ## empty value is no value, and RFC 5545 PRIORITY:0 means "undefined + ## priority" - plann treats it that way everywhere else, so those + ## tasks must still be offered for editing. + return searcher.check_component(obj) or not obj.icalendar_component.get(prop) -def _edit(ctx, add_category=None, cancel=None, interactive_ical=False, interactive_relations=False, mass_interactive_default='ignore', mass_interactive=False, interactive=False, complete=None, complete_recurrence_mode='safe', postpone=None, postpone_with_children=None, interactive_reprioritize=False, **kwargs): + return [obj for obj in objs if _missing(obj)] + +def _edit(ctx, cancel=None, interactive_ical=False, interactive_relations=False, mass_interactive_default='ignore', mass_interactive=False, interactive=False, complete=None, complete_recurrence_mode='safe', postpone=None, postpone_with_children=None, interactive_reprioritize=False, **kwargs): """ Edits a task/event/journal """ ## TODO: consolidate with command_edit if 'recurrence_mode' in kwargs: complete_recurrence_mode = kwargs.pop('recurrence_mode') + add_args = _process_add_args(kwargs) _process_set_args(ctx, kwargs, keep_category=True) if interactive_ical: _interactive_ical_edit(ctx.obj['objs']) @@ -280,15 +304,11 @@ def _edit(ctx, add_category=None, cancel=None, interactive_ical=False, interacti _interactive_edit(obj) comp = obj.icalendar_component if kwargs.get('pdb'): - click.echo("icalendar component available as comp") - click.echo("caldav object available as obj") - click.echo("do the necessary changes and press c to continue normal code execution") - click.echo("happy hacking") - breakpoint() + _pdb_edit(obj) for arg in ctx.obj['set_args']: _set_something(obj, arg, ctx.obj['set_args'][arg]) - if add_category: - _add_category(obj, add_category) + for canonical, tokens in add_args: + _add_comma_list(obj, canonical, tokens) if complete: obj.complete(handle_rrule=complete_recurrence_mode, rrule_mode=complete_recurrence_mode) elif complete is False: @@ -313,7 +333,7 @@ def _check_for_panic(ctx, hours_per_day, output=True, print_timeline=True, fix_t timeline_end = parse_dt(timeline_end, datetime.datetime) if include_all_events: ## Remove events from the list to prevent duplicates ... - ctx.obj['objs'] = [x for x in ctx.obj['objs'] if 'BEGIN:VEVENT' not in x.data] + ctx.obj['objs'] = [x for x in ctx.obj['objs'] if _component_type(x) != 'VEVENT'] ## ... and then add all events _select(ctx, event=True, start=timeline_start, end=timeline_end, extend_objects=True) possible_timeline = timeline_suggestion(ctx, hours_per_day=hours_per_day, timeline_end=timeline_end) @@ -376,6 +396,20 @@ def summary(obj): return possible_timeline +def _process_add_args(kwargs): + """Pop the --add- options out of kwargs and return a list of + ``(canonical_property, tokens)`` pairs to append. Only comma-list + properties (categories, resources) have add-options; the plural form splits + on comma, the singular form keeps the comma literal.""" + ret = [] + for key in [k for k in kwargs if k.startswith('add_')]: + value = kwargs.pop(key) + attr = key[4:] + if not value or not _is_comma_list_attr(attr): + continue + ret.append((_comma_list_canonical(attr), _comma_list_tokens(attr, value))) + return ret + def _process_set_args(ctx, kwargs, keep_category=False): ctx.obj['set_args'] = {} for x in kwargs: @@ -550,40 +584,36 @@ def _set_task_attribs(ctx): """ actual implementation of set_task_attribs """ - ## Tasks missing a category LIMIT = 16 + ## Fetch the pending todos once, then filter client-side per attribute - + ## rather than issuing a fresh server select for each attribute (E4). + _select(ctx=ctx, todo=True, sort_key=['{DTSTART:?{DUE:?(0000)?}?%F %H:%M:%S}', '{PRIORITY:?0?}']) + todos = list(ctx.obj['objs']) + def _set_something_(something, help_text, help_url=None, default=None, objs=None): - cond = {f"no_{something}": True} - something_ = 'categories' if something == 'category' else something + something_ = _comma_list_canonical(something) if _is_comma_list_attr(something) else something if something == 'duration': something_ = 'dtstart' - cond['no_dtstart'] = True - _select(ctx=ctx, todo=True, limit=LIMIT, sort_key=['{DTSTART:?{DUE:?(0000)?}?%F %H:%M:%S}', '{PRIORITY:?0?}'], **cond) - ## Doing some client-side filtering due to calendar servers that don't support the RFC properly - ## TODO: "Incompatibility workarounds" should be moved to the caldav library - objs_ = [x for x in ctx.obj['objs'] if not x.icalendar_component.get(something_)] + objs_ = _todos_missing(todos, something_) ## add all non-duplicated objects from objs to objs_ uids_ = {x.icalendar_component['UID'] for x in objs_} for obj in objs or []: if obj.icalendar_component['UID'] not in uids_: - obj.load() objs_.append(obj) objs = objs_ + if something == 'duration': + objs = [x for x in objs if x.icalendar_component.get('due')] if objs: - if something == 'duration': - objs = [x for x in objs if x.icalendar_component.get('due')] - num = len(objs) - if num == LIMIT: - num = f"{LIMIT} or more" + capped = len(objs) > LIMIT + objs = objs[:LIMIT] + num = f"{LIMIT} or more" if capped else len(objs) click.echo(f"There are {num} tasks with no {something} set.") click.echo(help_url) if something == 'category': - _select(ctx=ctx, todo=True) - cats = list(_cats(ctx)) - cats.sort() + cats = sorted(_categories_in_use(todos)) click.echo("List of existing categories in use (if any):") click.echo("\n".join(cats)) click.echo(f"For each task, {help_text}") @@ -596,8 +626,8 @@ def _set_something_(something, help_text, help_url=None, default=None, objs=None obj.complete() obj.save() continue - if something == 'category': - comp.add(something_, value.split(',')) + if _is_comma_list_attr(something): + comp.add(something_, _comma_list_tokens(something, value)) elif something == 'due': _procrastinate([obj], parse_dt(value, datetime.datetime), check_dependent='interactive', err_callback=click.echo, confirm_callback=click.confirm) elif something == 'duration': diff --git a/plann/config.py b/plann/config.py index 70ae81d..006938f 100644 --- a/plann/config.py +++ b/plann/config.py @@ -2,20 +2,71 @@ import logging import os import time -from fnmatch import fnmatch from getpass import getpass -import yaml - - -def interactive_config(args, config, remaining_argv): - - section = 'default' +## The config handling logic originated in plann and has been adopted by the +## caldav library - use the caldav implementation rather than carrying a copy. +## (The redundant aliases mark config_section and expand_config_section as +## intentional re-exports, they are used by cli.py) +from caldav.config import config_section as config_section +from caldav.config import expand_config_section as expand_config_section +from caldav.config import read_config as _read_config + +try: + from caldav.config import CONNKEYS as _CONNKEYS +except ImportError: ## older caldav releases don't export CONNKEYS + _CONNKEYS = None + +## Connection parameters are written caldav_-prefixed; caldav's reader +## (extract_conn_params_from_section) maps caldav_user/caldav_pass to +## username/password. Deriving the prompt list from caldav.config.CONNKEYS +## (rather than hardcoding an independent list) keeps the writer from drifting +## away from the reader - the historical bug was writing `ssl_verify_cert` +## without the caldav_ prefix, which the reader silently dropped (code review +## C8). +_CONN_ALIASES = {'user': 'username', 'pass': 'password'} +_CONN_PROMPT_KEYS = ('caldav_url', 'caldav_user', 'caldav_pass', 'caldav_proxy', 'caldav_ssl_verify_cert') + +if _CONNKEYS is not None: + ## fail fast if a prompt key stops mapping to a real caldav connection + ## parameter - i.e. if this list ever drifts from the reader again + for _k in _CONN_PROMPT_KEYS: + _bare = _k[len('caldav_'):] + assert _CONN_ALIASES.get(_bare, _bare) in _CONNKEYS, \ + f"prompt key {_k!r} is not a caldav connection parameter" + +## calendar_url/calendar_name are read directly by find_calendars; features is +## the caldav server-compatibility profile; inherits is the config meta-key +## resolved by config_section. (language/timezone are intentionally NOT +## prompted for - plann does not read them from the config file.) +CONFIG_PROMPT_KEYS = _CONN_PROMPT_KEYS + ('calendar_url', 'calendar_name', 'features', 'inherits') + + +def _prompt_value(label, current, secret=False): + if secret: + print(f"Config option {label} - old value: **HIDDEN**") + return getpass(prompt="Enter new value (or just enter to keep the old): ") + print("Config option {} - old value: {}".format(label, current if current is not None else '(None)')) + return input("Enter new value (or just enter to keep the old): ") + + +def interactive_config(config, config_file, config_section='default', allow_use=False): + """Interactively edit a configuration section and optionally save it. + + EXPERIMENTAL / under-tested - see the disclaimer printed at runtime. + + `config` is the parsed config dict (may be empty), `config_file` the path + to write to, `config_section` the section to edit, and `allow_use` whether + to offer using the config without saving (only meaningful when a follow-up + command will consume the returned config). Returns the modified config. + """ + section = config_section backup = {} modified = False print("Welcome to the interactive calendar configuration mode") - print("Warning - untested code ahead, raise issues at config-issues@plann.no or the github issue tracker") + print("WARNING - here be dragons: this interactive configuration is under-tested.") + print("Please raise issues at config-issues@plann.no or the github issue tracker.") print("It might be a good idea to read the documentation in parallel if running this for your first time") if not config or not hasattr(config, 'keys'): config = {} @@ -23,8 +74,8 @@ def interactive_config(args, config, remaining_argv): if config: print("The following sections have been found: ") print("\n".join(config.keys())) - if args.config_section and args.config_section != 'default': - section = args.config_section + if config_section and config_section != 'default': + section = config_section else: ## TODO: tab completion section = input("Chose one of those, or a new name / no name for a new configuration section: ") @@ -37,19 +88,21 @@ def interactive_config(args, config, remaining_argv): if section not in config: config[section] = {} - for config_key in ('caldav_url', 'calendar_url', 'caldav_user', 'caldav_pass', 'caldav_proxy', 'ssl_verify_cert', 'language', 'timezone', 'inherits'): - - if config_key == 'caldav_pass': - print("Config option caldav_pass - old value: **HIDDEN**") - value = getpass(prompt="Enter new value (or just enter to keep the old): ") - else: - print("Config option {} - old value: {}".format(config_key, config[section].get(config_key, '(None)'))) - value = input("Enter new value (or just enter to keep the old): ") - + sect = config[section] + for config_key in CONFIG_PROMPT_KEYS: + value = _prompt_value(config_key, sect.get(config_key), secret=(config_key == 'caldav_pass')) if value: - config[section][config_key] = value + sect[config_key] = value modified = True + ## time_tracking is non-connection config; find_calendars passes the whole + ## extra_config sub-dict through to add_time_tracking (see lib.py) + tt_current = sect.get('extra_config', {}).get('time_tracking') + tt_value = _prompt_value('time_tracking (e.g. timewarrior)', tt_current) + if tt_value: + sect.setdefault('extra_config', {})['time_tracking'] = tt_value + modified = True + if not modified: print("No configuration changes have been done") else: @@ -60,7 +113,7 @@ def interactive_config(args, config, remaining_argv): options.append(('save', f'save configuration into section {section}')) if backup or not section: options.append(('save_other', 'add this new configuration into a new section in the configuration file')) - if remaining_argv: + if allow_use: options.append(('use', 'use this configuration without saving')) options.append(('abort', 'abort without saving')) print("CONFIGURATION DONE ...") @@ -79,9 +132,9 @@ def interactive_config(args, config, remaining_argv): del config[section] section = new_section try: - if os.path.isfile(args.config_file): - os.rename(args.config_file, f"{args.config_file}.{int(time.time())}.bak") - with open(args.config_file, 'w') as outfile: + if os.path.isfile(config_file): + os.rename(config_file, f"{config_file}.{int(time.time())}.bak") + with open(config_file, 'w') as outfile: json.dump(config, outfile, indent=4) except Exception as e: print(e) @@ -89,85 +142,18 @@ def interactive_config(args, config, remaining_argv): print("Saved config") state = 'done' - if args.config_section == 'default' and section != 'default': + if config_section == 'default' and section != 'default': config['default'] = config[section] return config -## TODO TODO TODO - write test code for all the corner cases -## TODO TODO TODO - write documentation of config format -def expand_config_section(config, section='default', blacklist=None): +def read_config(fn, interactive_error=False): """ - In the "normal" case, will return [ section ] - - We allow: - - * * includes all sections in config file - * "Meta"-sections in the config file with the keyword "contains" followed by a list of section names - * Recursive "meta"-sections - * Glob patterns (work_* for all sections starting with work_) - * Glob patterns in "meta"-sections + Thin wrapper around the caldav library's read_config. The caldav + version raises ValueError on a broken config file - plann should + rather log the problem and carry on. """ - ## Optimizating for a special case. The results should be the same without this optimization. - if section == '*': - return [x for x in config if not config[x].get('disable', False)] - - ## If it's not a glob-pattern ... - if set(section).isdisjoint(set('[*?')): - ## If it's referring to a "meta section" with the "contains" keyword - if 'contains' in config[section]: - results = [] - if not blacklist: - blacklist = set() - blacklist.add(section) - for subsection in config[section]['contains']: - if subsection not in results and subsection not in blacklist: - for recursivesubsection in expand_config_section(config, subsection, blacklist): - if recursivesubsection not in results: - results.append(recursivesubsection) - return results - else: - ## Disabled sections should be ignored - if config.get('section', {}).get('disable', False): - return [] - - ## NORMAL CASE - return [ section ] - return [ section ] - ## section name is a glob pattern - matching_sections = [x for x in config if fnmatch(x, section)] - results = set() - for s in matching_sections: - if set(s).isdisjoint(set('[*?')): - results.update(expand_config_section(config, s)) - else: - ## Section names shouldn't contain []?* ... but in case they do ... don't recurse - results.add(s) - return results - -def config_section(config, section='default'): - if section in config and 'inherits' in config[section]: - ret = config_section(config, config[section]['inherits']) - else: - ret = {} - if section in config: - ret.update(config[section]) - return ret - -def read_config(fn, interactive_error=False): - ## This can probably be refactored into fewer lines ... try: - try: - with open(fn, 'rb') as config_file: - return json.load(config_file) - except json.decoder.JSONDecodeError: - try: - with open(fn, 'rb') as config_file: - return yaml.load(config_file, yaml.Loader) - except yaml.scanner.ScannerError: - logging.error(f"config file {fn!r} exists but is neither valid json nor yaml. Check the syntax.") - - except FileNotFoundError: - ## File not found - logging.info("no config file found") + return _read_config(fn) or {} except ValueError: if interactive_error: logging.error("error in config file. Be aware that the interactive configuration will ignore and overwrite the current broken config file", exc_info=True) diff --git a/plann/interactive.py b/plann/interactive.py index e099497..7c9dbd3 100644 --- a/plann/interactive.py +++ b/plann/interactive.py @@ -16,6 +16,7 @@ import os import re +import shutil import subprocess import tempfile @@ -24,6 +25,7 @@ from plann.lib import ( _adjust_relations, + _component_type, _list, _now, _process_set_arg, @@ -35,9 +37,18 @@ add_time_tracking, ) from plann.template import Template -from plann.timespec import _ensure_ts, parse_add_dur +from plann.timespec import DURATION_UNITS, _ensure_ts, parse_add_dur +def _pdb_edit(obj, interactive=True): + comp = obj.icalendar_component # noqa: F841 — visible in pdb session + if interactive: + click.echo("icalendar component available as comp") + click.echo("caldav object available as obj") + click.echo("do the necessary changes and press c to continue normal code execution") + click.echo("happy hacking") + breakpoint() + def command_edit(obj, command, interactive=True): if command == 'ignore': return @@ -60,8 +71,8 @@ def command_edit(obj, command, interactive=True): with_params['with_family'] = true if 'with children' in command: with_params['with_children'] = true - if 'with family' in command: - with_params['with_family'] = true + if 'with parent' in command: + with_params['with_parent'] = true ## TODO: we probably shouldn't be doing this interactively here? _procrastinate([obj], command.split(' ')[1], **with_params) elif command == 'complete': @@ -86,12 +97,7 @@ def command_edit(obj, command, interactive=True): ## TODO - experimental and very incomplete! add_time_tracking(obj) elif command == 'pdb': - if interactive: - click.echo("icalendar component available as comp") - click.echo("caldav object available as obj") - click.echo("do the necessary changes and press c to continue normal code execution") - click.echo("happy hacking") - breakpoint() + _pdb_edit(obj, interactive=interactive) else: if interactive: click.echo(f"unknown instruction '{command}' - ignoring") @@ -150,13 +156,6 @@ def count_indent(line): return j return None - def get_obj(line): - """Check the uuid on the line and return the caldav object""" - uid = line.lstrip().split(':')[0] - if not uid: - raise NotImplementedError("No uid - what now?") - return calendar.object_by_uid(uid) - i=0 children = [] while id / ignore / part(ially-complete) / complete / split / cancel / set foo=bar / edit / family / pdb?", default='ignore') - command_edit(obj, input, interactive=True) + input = click.prompt("postpone d / ignore / part(ially-complete) / complete / split / cancel / set foo=bar / edit / family / start / pdb?", default='ignore') + try: + command_edit(obj, input, interactive=True) + except NotImplementedError as e: + ## e.g. `start` when extra_config.time_tracking is not configured - + ## the prompt advertises the command, so report why it did not work + ## rather than tearing down the rest of the session with a traceback + click.echo(f"Could not run '{input}': {e}") + return + if input == 'start': + ## time tracking has been started - re-prompt so a follow-up command can be given for the same task + _interactive_edit(obj) def _mass_reprioritize(objs): text = """\ @@ -295,7 +305,7 @@ def _mass_interactive_edit(objs, default='ignore'): def interactive_split_task(obj, partially_complete=False, too_big=True): comp = obj.icalendar_component - summary = comp.get('summary') or comp.get('description') or comp.get('uid') + summary = _summary(obj) estimate = obj.get_duration() tbm = "" if too_big: @@ -330,7 +340,7 @@ def interactive_split_task(obj, partially_complete=False, too_big=True): new_summary = click.prompt("Summary of the parent task?", default=obj.icalendar_component['SUMMARY']) obj.icalendar_component['SUMMARY'] = new_summary postpone = click.prompt("Should we postpone the parent task?", default='0h') - if postpone in ('0h', '0'): ## TODO: regexp? + if postpone not in ('0h', '0'): ## TODO: regexp? _procrastinate([obj], postpone, check_dependent='interactive', err_callback=click.echo, confirm_callback=click.confirm) obj.save() @@ -338,16 +348,12 @@ def _editor(sometext): with tempfile.NamedTemporaryFile(mode='w', encoding='UTF-8', delete=False) as tmpfile: tmpfile.write(sometext) fn = tmpfile.name - editor = os.environ.get("VISUAL") or os.environ.get("EDITOR") or "" - if '/' not in editor: - for path in os.environ.get("PATH", "").split(os.pathsep): - full_path = os.path.join(path, editor) - if os.path.exists(full_path) and os.access(full_path, os.X_OK): - editor = full_path - break - for ed in (editor, '/usr/bin/vim', '/usr/bin/vi', '/usr/bin/emacs', '/usr/bin/nano', '/usr/bin/pico', '/bin/vi'): - if os.path.isfile(ed) and os.access(ed, os.X_OK): - break + candidates = (os.environ.get("VISUAL"), os.environ.get("EDITOR"), + 'vim', 'vi', 'emacs', 'nano', 'pico') + ed = next((found for c in candidates if c and (found := shutil.which(c))), None) + if not ed: + os.unlink(fn) + raise FileNotFoundError("no usable editor found; set the EDITOR or VISUAL environment variable") subprocess.run([ed, fn]) with open(fn) as tmpfile: ret = tmpfile.read() @@ -375,7 +381,7 @@ def _get_obj_from_line(line, calendar): return obj def _command_line_edit(line, calendar, interactive=True): - regexp = re.compile("((?:set [^ ]*=[^ ]*)|(?:postpone (?:[0-9]+[smhdwy]|20[0-9][0-9]-[0-9][0-9]-[0-9][0-9]))|[^ ]*) (.*)$") + regexp = re.compile(rf"((?:set [^ ]*=[^ ]*)|(?:postpone (?:[0-9]+[{DURATION_UNITS}]|20[0-9][0-9]-[0-9][0-9]-[0-9][0-9]))|[^ ]*) (.*)$") line = _strip_line(line) if not line: return diff --git a/plann/lib.py b/plann/lib.py index 0112d94..d6395f5 100644 --- a/plann/lib.py +++ b/plann/lib.py @@ -17,6 +17,11 @@ import click ## TODO - this should be removed, eventually import icalendar +try: + from caldav.config import extract_conn_params_from_section +except ImportError: ## caldav <= 3.2.1 has it as a private function + from caldav.config import _extract_conn_params_from_section as extract_conn_params_from_section + from plann.template import Template from plann.timespec import ( _ensure_ts, @@ -33,10 +38,81 @@ ## TODO: maybe find those attributes through the icalendar library? icalendar.cal.singletons, icalendar.cal.multiple, etc attr_txt_one = ['location', 'description', 'geo', 'organizer', 'summary', 'class', 'rrule', 'status'] -attr_txt_many = ['category', 'comment', 'contact', 'resources', 'parent', 'child'] ## category is an odd-ball, it should be categories - but we need a lot more test code before we can change that. +## NOTE: "category" (singular) looks like an odd-ball next to the plural +## "resources", but it is intentional: in a *search* the singular form means a +## substring match and the plural "categories" an exact match (this distinction +## is implemented in the caldav library / icalendar-searcher). For *editing*, +## the singular/plural pair is a comma-literal/comma-split convenience handled +## through COMMA_LIST_ATTRS below. +attr_txt_many = ['category', 'comment', 'contact', 'resources', 'parent', 'child'] attr_time = ['dtstamp', 'dtstart', 'due', 'dtend', 'duration'] attr_int = ['priority'] +## RFC 5545 "comma-token list" properties: multi-valued text properties whose +## value is a comma-separated list of short tokens. Exposed on the CLI in both +## plural (comma-split, exact) and singular (comma-literal, substring) form. +## Maps the plural canonical property name -> the singular alias. Adding a new +## such property here is all it takes for the edit machinery to handle it. +COMMA_LIST_ATTRS = { + 'categories': 'category', + 'resources': 'resource', +} +_COMMA_LIST_SINGULARS = {singular: plural for plural, singular in COMMA_LIST_ATTRS.items()} + +def _is_comma_list_attr(name): + return name in COMMA_LIST_ATTRS or name in _COMMA_LIST_SINGULARS + +def _comma_list_is_plural(name): + return name in COMMA_LIST_ATTRS + +def _comma_list_canonical(name): + """The plural canonical property name for a comma-list attr (either form).""" + return name if name in COMMA_LIST_ATTRS else _COMMA_LIST_SINGULARS[name] + +def _comma_list_tokens(name, value): + """Normalise a CLI/interactive value into a list of tokens. + + A bare string (interactive ``set cat=a,b``) is always split on comma. A + tuple/list (click ``multiple=True``) is split only for the *plural* form and + only when a lone value contains a comma - so ``--add-category a,b`` keeps the + literal ``a,b`` while ``--add-categories a,b`` yields ``a`` and ``b``. + """ + if hasattr(value, 'split'): + return value.split(',') + value = list(value) + if _comma_list_is_plural(name) and len(value) == 1 and ',' in value[0]: + return value[0].split(',') + return value + +def _comma_list_existing(comp, canonical): + """Existing tokens of a comma-list property as a plain list of strings. + + Handles icalendar storing CATEGORIES as a single ``vCategory`` (``.cats``) + but RESOURCES as a list of ``vText`` (one property per value). + """ + if canonical not in comp: + return [] + val = comp.pop(canonical) + if hasattr(val, 'cats'): + return [str(x) for x in val.cats] + if isinstance(val, list): + return [str(x) for x in val] + return [str(val)] + +def _add_comma_list(obj, canonical, tokens): + """Append ``tokens`` to a comma-list property (e.g. CATEGORIES, RESOURCES).""" + comp = _icalendar_component(obj) + existing = _comma_list_existing(comp, canonical) + existing.extend(tokens) + comp.add(canonical, existing) + +def _set_comma_list(obj, canonical, tokens): + """Replace a comma-list property with ``tokens``.""" + comp = _icalendar_component(obj) + if canonical in comp: + comp.pop(canonical) + comp.add(canonical, tokens) + def _split_vcal(ical): """ This method will take an ical string containing one VCALENDAR with multiple calendar resource objects and split it into one VCALENDAR per calendar resource object. @@ -62,94 +138,47 @@ def _split_vcal(ical): for tz in ical_cal_stripped.subcomponents: split_by_uid[uid].add_component(tz) split_by_uid[uid].add_component(subcomponent) - return split_by_uid.values() + ## Return ical strings, like _split_vcals does - the callers hand the + ## result on to _caldav_objclass()/add_object(), which parse text. + return [cal.to_ical().decode() for cal in split_by_uid.values()] def _split_vcals(ical): """ This method will take a string with multiple VCALENDAR entries and - split it into a list + split it into a list (one ical string per VCALENDAR). + + Delegates the parsing to icalendar (which understands CRLF line endings + and line folding) rather than scanning the raw string by hand. """ - ical = ical.strip() - icals = [] - while ical.startswith("BEGIN:VCALENDAR\n"): - pos = ical.find("\nEND:VCALENDAR") + 14 - icals.append(ical[:pos]) - ical = ical[pos:].lstrip() - return icals + return [cal.to_ical().decode() for cal in icalendar.Calendar.from_ical(ical, multiple=True)] def find_calendars(args, raise_errors): - def list_(obj): - """ - For backward compatibility, a string rather than a list can be given as - calendar_url, calendar_name. Make it into a list. - """ - if not obj: - obj = [] - if isinstance(obj, str) or isinstance(obj, bytes): - obj = [ obj ] - return obj - - def _try(meth, kwargs, errmsg): - try: - ret = meth(**kwargs) - assert(ret) - return ret - except Exception: - logging.error(f"Problems fetching calendar information: {errmsg} - skipping") - if raise_errors: - raise - else: - return None - - conn_params = {} - for k in args: - if k.startswith('caldav_') and args[k]: - key = k[7:] - if key == 'pass': - key = 'password' - if key == 'user': - key = 'username' - conn_params[key] = args[k] - # Pass features parameter directly to DAVClient if specified - if args.get('features'): - conn_params['features'] = args['features'] - extra_params = {} - if 'extra_params' in conn_params: - extra_params = conn_params.pop('extra_params') - calendars = [] - ## TODO: test this more thoroughly. - ## The code above is supposed to remote the `caldav_`-prefix - ## Stil the lines below was added to fix - ## https://github.com/pycalendar/plann/issues/11, credits to @bergercookie - if 'caldav_url' in conn_params: - conn_params['url'] = conn_params.pop('caldav_url') - if conn_params: - client = caldav.DAVClient(**conn_params) - principal = _try(client.principal, {}, conn_params['url']) - if not principal: - return [] - calendars = [] - tries = 0 - for calendar_url in list_(args.get('calendar_url')): - if '/' in calendar_url: - calendar = principal.calendar(cal_url=calendar_url) - else: - calendar = principal.calendar(cal_id=calendar_url) - tries += 1 - if _try(calendar.get_display_name, {}, calendar.url): - calendars.append(calendar) - for calendar_name in list_(args.get('calendar_name')): - tries += 1 - calendar = _try(principal.calendar, {'name': calendar_name}, '{} : calendar "{}"'.format(conn_params['url'], calendar_name)) - calendars.append(calendar) - if not calendars and tries == 0: - calendars = _try(principal.calendars, {}, "conn_params['url'] - all calendars") - - if extra_params: - for cal in calendars: - cal.extra_params = extra_params - - return calendars or [] + """ + Find calendars from a dict of connection parameters - typically a config + file section or the command line arguments. The connection keys are + caldav_-prefixed (caldav_url, caldav_user, caldav_pass, ...), optionally + accompanied by `features` and the calendar_url/calendar_name filters. + + Connection parameter extraction (including resolving the URL from a + `features` server profile when no caldav_url is given) and the calendar + lookup itself are delegated to the caldav library. + """ + conn_params = extract_conn_params_from_section(args) + if not conn_params: + return [] + calendars = caldav.get_calendars( + check_config_file=False, + environment=False, + raise_errors=raise_errors, + calendar_url=args.get('calendar_url'), + calendar_name=args.get('calendar_name'), + **conn_params, + ) + ## Non-connection configuration (i.a. the time tracking integration, + ## cf. add_time_tracking) is attached to the calendar objects + for cal in calendars: + cal.extra_config = args.get('extra_config', {}) + return calendars def _icalendar_component(obj): try: @@ -158,23 +187,40 @@ def _icalendar_component(obj): ## assume obj is an icalendar_component return obj +def _component_type(obj): + """Return the iCalendar component name ('VEVENT', 'VTODO', 'VJOURNAL', ...) + for a caldav object or icalendar component. + + Preferred over sniffing 'BEGIN:VEVENT' etc. in the raw .data, which also + matches the substring inside a description/summary text body. + """ + return _icalendar_component(obj).name + +def _caldav_objclass(ical): + """Map a single iCalendar object (raw text) to its caldav class, parsing + it properly rather than substring-sniffing 'BEGIN:VTODO' etc. in the body. + """ + classes = {'VTODO': caldav.Todo, 'VJOURNAL': caldav.Journal, 'VEVENT': caldav.Event} + for comp in icalendar.Calendar.from_ical(ical).subcomponents: + if comp.name in classes: + return classes[comp.name] + return caldav.Event + def _add_category(obj, category): - comp = _icalendar_component(obj) - if 'categories' in comp: - cats = comp.pop('categories').cats - else: - cats = [] - if hasattr(category, 'split'): - category = category.split(',') - cats.extend(category) - comp.add('categories', cats) + """Append one or more categories. + + Back-compat wrapper around the generic comma-list helper; ``category`` may + be a comma-separated string or a list/tuple of categories. + """ + tokens = category.split(',') if hasattr(category, 'split') else list(category) + _add_comma_list(obj, 'categories', tokens) def add_time_tracking_timew(obj, start=None, end=None): comp = _icalendar_component(obj) tags = ['plann-export'] if 'categories' in comp: - for cat in comp.pop('categories').cats: + for cat in comp['categories'].cats: tags.append(f'category:{cat}') tags.append(f'uid:{comp["uid"]}') tags.append(f'summary:{_summary(obj)}') @@ -193,12 +239,8 @@ def add_time_tracking_timew(obj, start=None, end=None): subprocess.run(["timew", "start"] + tags) def add_time_tracking(obj, start=None, end=None): - time_tracking = None comp = _icalendar_component(obj) - if hasattr(obj.parent, 'extra_config'): - cfg = obj.parent.extra_config - if 'time_tracking' in cfg: - time_tracking = cfg['time_tracking'] + time_tracking = getattr(obj.parent, 'extra_config', {}).get('time_tracking') if time_tracking is None: raise NotImplementedError('Time tracking is so far not supported internally in plann, only through external tools, and only timewarrior as for now. You have to set `time_tracking=timewarrior` in your calendar configuration') @@ -207,9 +249,11 @@ def add_time_tracking(obj, start=None, end=None): start = comp.start end = comp.end + if isinstance(time_tracking, str): + time_tracking = [time_tracking] for tt in time_tracking: ## TODO: this must be done in a more clever way if introducing more time tracking types - if tt == 'timew': + if tt in ('timewarrior', 'Timewarrior', 'timew'): add_time_tracking_timew(obj, start, end) else: raise NotImplementedError('Only time tracking through taskw supported so far') @@ -283,10 +327,6 @@ def _procrastinate(objs, delay, check_dependent="error", with_children=False, wi else: err_callback(f"{summary} could not be postponed due to parent {_summary(p)} with due {_ensure_ts(p['DUE'])} and priority {p.get('priority', 0)}") if p_postponable and (p_auto_postponable or confirm_callback("procrastinate parent?")): - import inspect - stack_depth = len(inspect.stack()) - if stack_depth > 13: - breakpoint() _procrastinate([parent], new_due+max(parent.get_duration()+x.get_duration()+datetime.timedelta(minutes=1), datetime.timedelta(minutes=1)), check_dependent=check_dependent, err_callback=err_callback, confirm_callback=confirm_callback, recursivity=recursivity+1) _procrastinate([x], new_due, check_dependent=check_dependent, err_callback=err_callback, confirm_callback=confirm_callback, recursivity=recursivity+1) elif check_dependent == "return": @@ -378,14 +418,55 @@ def _adjust_relations(parent, children): parent.save() _remove_reverse_relations(parent, pmutated['removed']) +class _RelativeCache: + """Per-traversal cache for relationship listing. + + A hierarchical ``list --top-down`` walks the parent/child graph and would + otherwise re-fetch the same task from the server once per edge (a parent + is fetched again for every child, and again on every recursion step): + ~N×R round-trips for N tasks with R relations each (code review E3). + + This caches both the object-by-UID lookups and the per-object relationship + scan, so each task is fetched - and its consistency-checked - at most once + for the whole traversal. + """ + def __init__(self): + self._objects = {} + self._relships = {} + + def get_object(self, calendar, uid): + key = (getattr(calendar, 'url', None), str(uid)) + if key not in self._objects: + self._objects[key] = calendar.get_object_by_uid(uid) + return self._objects[key] + + def cached_relships(self, obj, reltype_wanted): + return self._relships.get((str(obj.icalendar_component['UID']), reltype_wanted)) + + def store_relships(self, obj, reltype_wanted, relships): + self._relships[(str(obj.icalendar_component['UID']), reltype_wanted)] = relships + ## TODO: As for now, this one will throw the user into the python debugger if inconsistencies are found. ## It for sure cannot be like that when releasing plann 1.0! -def _relships_by_type(obj, reltype_wanted=None): +def _relships_by_type(obj, reltype_wanted=None, cache=None): + if cache is None: + cache = _RelativeCache() + cached = cache.cached_relships(obj, reltype_wanted) + if cached is not None: + return cached + backreltypes = {'CHILD': 'PARENT', 'PARENT': 'CHILD', 'undefined': 'CHILD', 'SIBLING': 'SIBLING'} - rels_by_type = obj.get_relatives(reltype_wanted) + ## Parse the related UIDs straight from obj's ical (no network) and resolve + ## each one through the cache, rather than letting caldav fetch them anew. + rels_by_type = obj.get_relatives(reltype_wanted, fetch_objects=False) ret = defaultdict(list) for reltype in rels_by_type: - for other in rels_by_type[reltype]: + for other_uid in rels_by_type[reltype]: + try: + other = cache.get_object(obj.parent, other_uid) + except caldav.error.NotFoundError: + ## a dangling relation - mirrors get_relatives(ignore_missing=True) + continue ret[reltype].append(other) ## Consistency check ... TODO ... look more into breakages @@ -397,7 +478,7 @@ def _relships_by_type(obj, reltype_wanted=None): back_rel_types.add(back_rel_type) if len(back_rel_types) > 1: - logging.error(f"Inconsistency issue in relationships - has to be manually resolved (UID={obj.icalendar_component_UID}, backrels: {back_rel_types})") + logging.error(f"Inconsistency issue in relationships - has to be manually resolved (UID={obj.icalendar_component['UID']}, backrels: {back_rel_types})") ## Inconsistency has to be manually fixed: more than one related-to property pointing from other to obj if len(back_rel_types) == 0: logging.error("Inconsistency issue in relationships - will be automatically fixed: no related-to property pointing from other to obj") @@ -407,6 +488,7 @@ def _relships_by_type(obj, reltype_wanted=None): else: if back_rel_types != { backreltypes[reltype] }: logging.error("Inconsistency issue in relationships - has to be manually resolved. Object and other points to each other, but reltype does not match") + cache.store_relships(obj, reltype_wanted, ret) return ret def _relationship_text(obj, reltype_wanted=None): @@ -419,7 +501,7 @@ def _relationship_text(obj, reltype_wanted=None): for relobj in rels[reltype]: objs.append(_summary(relobj)) ret.append(reltype + "\n" + "\n".join(objs) + "\n") - return "\n".join(ret) + return "\n".join(ret) ## TODO - this needs to be better documented. What's the difference between _process_set_arg and _set_something? Do they overlap? Are they intended to be used together? def _process_set_arg(arg, value, keep_category=False): @@ -434,13 +516,13 @@ def _process_set_arg(arg, value, keep_category=False): k,v = split1.split('=') rrule[k] = v ret[arg] = rrule - elif arg in ('category', 'categories'): - if hasattr(value, 'split'): - value = value.split(',') - elif len(value) == 1 and arg == 'categories' and ',' in value[0]: - value = value[0].split(',') - if not keep_category: - arg = 'categories' + elif _is_comma_list_attr(arg): + value = _comma_list_tokens(arg, value) + ## Without keep_category the singular alias is canonicalised to its + ## plural (replace) form - used by the create path, which forwards + ## set_args straight to caldav's save_todo(categories=...) etc. + if not keep_category and not _comma_list_is_plural(arg): + arg = _comma_list_canonical(arg) ret[arg] = value else: ret[arg] = value @@ -460,8 +542,14 @@ def _set_something(obj, arg, value): obj.set_duration(value) elif arg in ('due', 'dtend'): ## TODO: dtstart! getattr(obj, f"set_{arg}")(value, move_dtstart=True, check_dependent=True) - elif arg == 'category': - _add_category(obj, value) + elif _is_comma_list_attr(arg): + ## a list (already processed by _process_set_arg) or a raw comma string + tokens = _comma_list_tokens(arg, value) + canonical = _comma_list_canonical(arg) + if _comma_list_is_plural(arg): + _set_comma_list(obj, canonical, tokens) ## plural -> replace + else: + _add_comma_list(obj, canonical, tokens) ## singular -> append else: if arg in comp: comp.pop(arg) @@ -471,7 +559,7 @@ def _set_something(obj, arg, value): ## TODO: should be rewritten a bit, we should have a create_list method that does not call on click.echo directly ## let the caller decide if click is to be used or not. ## Use the yield method to avoid having to generate the full list prior to printing to screen -def _list(objs, ics=False, template="{DTSTART:?{DUE:?(date missing)?}?%F %H:%M:%S %Z}: {SUMMARY:?{DESCRIPTION:?(no summary given)?}?}", top_down=False, bottom_up=False, indent=0, echo=True, uids=None, filter=lambda obj: True): +def _list(objs, ics=False, template="{DTSTART:?{DUE:?(date missing)?}?%F %H:%M:%S %Z}: {SUMMARY:?{DESCRIPTION:?(no summary given)?}?}", top_down=False, bottom_up=False, indent=0, echo=True, uids=None, filter=lambda obj: True, separator="\n", cache=None): """ Actual implementation of list @@ -480,13 +568,16 @@ def _list(objs, ics=False, template="{DTSTART:?{DUE:?(date missing)?}?%F %H:%M:% """ if indent>32: raise NotImplementedError("too deep hierarchies, or circular links") + ## one relationship cache for the whole (recursive) traversal, so the same + ## task is not re-fetched from the server for every edge it touches (E3) + if cache is None and (top_down or bottom_up): + cache = _RelativeCache() if ics: - if not objs: + accepted = [obj for obj in objs if filter(obj)] + if not accepted: return - icalendar = objs.pop(0).icalendar_instance - for obj in objs: - if not filter(obj): - continue + icalendar = accepted[0].icalendar_instance + for obj in accepted[1:]: icalendar.subcomponents.extend(obj.icalendar_instance.subcomponents) click.echo(icalendar.to_ical()) return @@ -513,7 +604,7 @@ def _list(objs, ics=False, template="{DTSTART:?{DUE:?(date missing)?}?%F %H:%M:% above = [] below = [] if top_down or bottom_up: - relations = _relships_by_type(obj) + relations = _relships_by_type(obj, cache=cache) parents = relations['PARENT'] children = relations['CHILD'] ## in a top-down view, the (grand)*parent should be shown as a top-level item rather than the object. @@ -534,17 +625,17 @@ def _list(objs, ics=False, template="{DTSTART:?{DUE:?(date missing)?}?%F %H:%M:% more_info['calendar_url'] = obj.parent.url output.append(" "*indent + template.format(**obj.icalendar_component, **more_info)) ## Recursively add children in an indented way - output.extend(_list(below, template=template, top_down=top_down, bottom_up=bottom_up, indent=indent+2, echo=False, filter=filter)) + output.extend(_list(below, template=template, top_down=top_down, bottom_up=bottom_up, indent=indent+2, echo=False, filter=filter, cache=cache)) if indent and top_down: ## Include all siblings as same-level nodes ## Use the top-level uids to avoid infinite recursion ## TODO: siblings are probably not being handled correctly here. Should write test code and investigate. - output.extend(_list(relations['SIBLING'], template=template, top_down=top_down, bottom_up=bottom_up, indent=indent, echo=False, uids=uids, filter=filter)) + output.extend(_list(relations['SIBLING'], template=template, top_down=top_down, bottom_up=bottom_up, indent=indent, echo=False, uids=uids, filter=filter, cache=cache)) for p in above: ## The item should be part of a sublist. Find and add the top-level item, and the full indented list under there - recursively. puid = p.icalendar_component['UID'] if puid not in uids: - output.extend(_list([p], template=template, top_down=top_down, bottom_up=bottom_up, indent=indent, echo=False, uids=uids, filter=filter)) + output.extend(_list([p], template=template, top_down=top_down, bottom_up=bottom_up, indent=indent, echo=False, uids=uids, filter=filter, cache=cache)) if echo: - click.echo_via_pager("\n".join(output)) + click.echo_via_pager(separator.join(output)) return output diff --git a/plann/panic_planning.py b/plann/panic_planning.py index e6e2c5b..f7bec8f 100644 --- a/plann/panic_planning.py +++ b/plann/panic_planning.py @@ -2,7 +2,7 @@ from sortedcontainers import SortedKeyList -from plann.lib import _ensure_ts, _now +from plann.lib import _component_type, _ensure_ts, _now class TimeLine(SortedKeyList): @@ -103,23 +103,22 @@ def pad_slack(self, end, duration): def timeline_suggestion(ctx, hours_per_day=4, timeline_end=None): timeline = TimeLine() objs = ctx.obj['objs'] - events = [x for x in objs if 'BEGIN:VEVENT' in x.data] + events = [x for x in objs if _component_type(x) == 'VEVENT'] event_parents = [] for event in events: comp = event.icalendar_component if comp.get('STATUS', '') == 'CANCELLED': continue - if 'RELATED-TO' in comp and event.get_dtend()>_now(): + dtend = event.get_dtend() + if 'RELATED-TO' in comp and dtend is not None and _ensure_ts(dtend)>_now(): rels = event.get_relatives(fetch_objects=False) for rel in rels['PARENT']: event_parents.append(str(rel)) - tasks = [x for x in objs if 'BEGIN:VTODO' in x.data] + tasks = [x for x in objs if _component_type(x) == 'VTODO'] assert len(events) + len(tasks) == len(objs) tasks = [x for x in tasks if ('\nDUE' in x.data or '\nDURATION' in x.data) and '\nDTSTART' in x.data] for event in events: - if 'BEGIN:VEVENT' not in event.data: - continue ## TODO ... we should handle overlapping events a bit better than just ignoring AssertionErrors try: timeline.add_event(event) diff --git a/plann/timespec.py b/plann/timespec.py index f991f53..ea848a6 100644 --- a/plann/timespec.py +++ b/plann/timespec.py @@ -4,6 +4,7 @@ from dataclasses import dataclass import dateparser +from dateutil.relativedelta import relativedelta """ Most important content: @@ -18,6 +19,21 @@ The naming of those two are a bit arbitrary and may be changed in a future version of the library. Old names will then continue working as legacy aliases. """ +## The relative-duration mini-grammar (e.g. "2h", "1y1w", "+2.5h"). Historically +## this [smhdwy] unit set was duplicated in four places (parse_add_dur, +## _parse_timespec, commands.__select and interactive._command_line_edit); it lives +## here now so adding a new unit (e.g. months) only touches one spot. +DURATION_UNITS = "smhdwy" +## A single magnitude+unit token (signed/decimal), with the remainder captured in +## group 3 so callers can tokenize a multi-unit duration one component at a time. +DURATION_TOKEN_RE = re.compile(rf'([+-]?\d+(?:\.\d+)?)([{DURATION_UNITS}])(.*)') +## A complete bare duration such as "1y1w2h" (one capturing group around the whole). +DURATION_RE = re.compile(rf'((?:\d+(?:\.\d+)?[{DURATION_UNITS}])+)') + +def is_duration(text): + """True if the whole string is a bare relative duration like "1y1w2h".""" + return bool(re.fullmatch(DURATION_RE, text)) + ## Singleton (aka global variable) @dataclass class Tz: @@ -148,10 +164,11 @@ def parse_add_dur(dt, dur, for_storage=False, ts_allowed=False): time_units = { 's': 1, 'm': 60, 'h': 3600, 'd': 86400, 'w': 604800, - 'y': 1314000 + 'y': 31536000 } + diff = datetime.timedelta(0) while dur: - rx = re.match(r'([+-]?\d+(?:\.\d+)?)([smhdwy])(.*)', dur) + rx = DURATION_TOKEN_RE.match(dur) if not rx: if ts_allowed: return parse_dt(dur) @@ -160,12 +177,16 @@ def parse_add_dur(dt, dur, for_storage=False, ts_allowed=False): i = float(rx.group(1)) u = rx.group(2) dur = rx.group(3) - if u=='y' and dt: - dt = datetime.datetime.combine(datetime.date(dt.year+int(i), dt.month, dt.day), dt.time(), tzinfo=dt.tzinfo) + if u=='y': + if dt: + dt = dt + relativedelta(years=int(i)) + else: + diff += datetime.timedelta(seconds=int(i)*time_units['y']) else: - diff = datetime.timedelta(0, i*time_units[u]) + component = datetime.timedelta(0, i*time_units[u]) + diff += component if dt: - dt = dt + diff + dt = dt + component if dt: return dt.astimezone(tz.store_timezone) if for_storage else dt else: @@ -206,7 +227,7 @@ def _parse_timespec(timespec): ## calendar-cli format, 1998-10-03 15:00+2h if '+' in timespec: - rx = re.match(r'(.*)\+((?:\d+(?:\.\d+)?[smhdwy])+)$', timespec) + rx = re.match(rf'(.*)\+{DURATION_RE.pattern}$', timespec) if rx: start = parse_dt(rx.group(1)) end = parse_add_dur(start, rx.group(2)) @@ -225,5 +246,3 @@ def _parse_timespec(timespec): return (parse_dt(f"{split_by_space[0]} {split_by_space[1]}"), parse_dt(f"{split_by_space[2]} {split_by_space[3]}")) else: raise ValueError(f"couldn't parse time interval {timespec}") - - raise NotImplementedError("possibly a ISO time interval") diff --git a/pyproject.toml b/pyproject.toml index 8005c74..a1f2fc6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -24,12 +24,14 @@ classifiers = [ ] keywords = ["caldav", "calendar", "cli", "todo", "planning"] dependencies = [ - "caldav>=3.2.1", + "caldav>=3.3.0", "Click", "PyYAML", "sortedcontainers", "dateparser>=1.2", + "python-dateutil", "icalendar", + "icalendar_searcher", ] [project.urls] @@ -99,3 +101,8 @@ exclude_lines = [ [tool.deptry] pep621_dev_dependency_groups = ["dev"] + +[tool.deptry.per_rule_ignores] +## PyYAML is no longer imported directly, but it's still needed for yaml +## config files - the caldav library imports it lazily without declaring it +DEP002 = ["PyYAML"] diff --git a/tests/test_commands.py b/tests/test_commands.py new file mode 100644 index 0000000..6ffdd6d --- /dev/null +++ b/tests/test_commands.py @@ -0,0 +1,25 @@ +from unittest.mock import MagicMock, patch # noqa: F401 + +from caldav import Todo + + +def test_todos_missing_treats_priority_zero_as_undefined(): + """PRIORITY:0 is RFC 5545 "undefined priority", and plann treats it as + such everywhere else (`comp.get('PRIORITY', 0)`, _mass_reprioritize). + set-task-attribs must therefore still offer to set a priority on such a + task - filtering on "property absent" alone skips exactly the tasks the + feature exists to fix.""" + from plann.commands import _todos_missing + + def _todo(uid, extra): + return Todo(data=( + "BEGIN:VCALENDAR\nVERSION:2.0\nBEGIN:VTODO\n" + f"UID:{uid}\nSUMMARY:{uid}\n{extra}END:VTODO\nEND:VCALENDAR")) + + absent = _todo('no-priority', '') + zero = _todo('zero-priority', 'PRIORITY:0\n') + real = _todo('real-priority', 'PRIORITY:3\n') + + missing = _todos_missing([absent, zero, real], 'priority') + uids = {x.icalendar_component['UID'] for x in missing} + assert uids == {'no-priority', 'zero-priority'} diff --git a/tests/test_config.py b/tests/test_config.py new file mode 100644 index 0000000..2944a07 --- /dev/null +++ b/tests/test_config.py @@ -0,0 +1,87 @@ +"""Tests for plann.config.interactive_config. + +The interactive configuration writer must only emit keys that the caldav +config reader (extract_conn_params_from_section) and plann's find_calendars +actually consume - otherwise the prompt writes config that is silently +dropped at connect time (code review C8).""" + +import builtins + +from plann.config import interactive_config + +try: + from caldav.config import extract_conn_params_from_section +except ImportError: ## caldav <= 3.2.1 has it as a private function + from caldav.config import _extract_conn_params_from_section as extract_conn_params_from_section + + +def _drive(monkeypatch, inputs, secrets=None): + """Run interactive_config feeding `inputs` to input() and `secrets` to + getpass(), aborting at the save prompt. Returns the (mutated) config.""" + inputs = list(inputs) + secrets = list(secrets or []) + + def fake_input(prompt=""): + return inputs.pop(0) + + def fake_getpass(prompt=""): + return secrets.pop(0) + + monkeypatch.setattr(builtins, "input", fake_input) + monkeypatch.setattr("plann.config.getpass", fake_getpass) + + config = {} + return interactive_config(config, config_file="/nonexistent/never-written.conf") + + +def test_interactive_config_only_writes_consumable_keys(monkeypatch): + ## one value per prompt key, in order, then 'abort' at the save prompt + inputs = [ + "https://calendar.example.com/dav", # caldav_url + "user", # caldav_user + "http://proxy.example.com", # caldav_proxy + "true", # caldav_ssl_verify_cert + "https://calendar.example.com/cal", # calendar_url + "My Calendar", # calendar_name + "", # features (skip - would be resolved) + "", # inherits (skip) + "timewarrior", # extra_config.time_tracking + "abort", # save-state command + ] + config = _drive(monkeypatch, inputs, secrets=["hunter2"]) + section = config["default"] + + ## the ssl key must be caldav_-prefixed, otherwise caldav drops it + assert "caldav_ssl_verify_cert" in section + assert "ssl_verify_cert" not in section + + ## keys plann does not consume must not be written + assert "language" not in section + assert "timezone" not in section + + ## time tracking rides in the extra_config sub-dict find_calendars passes on + assert section["extra_config"]["time_tracking"] == "timewarrior" + + +def test_interactive_config_connection_keys_survive_extractor(monkeypatch): + """Everything the writer emits as a connection parameter must be picked up + by caldav's extractor - the C8 bug was that ssl_verify_cert was dropped.""" + inputs = [ + "https://calendar.example.com/dav", # caldav_url + "user", # caldav_user + "http://proxy.example.com", # caldav_proxy + "true", # caldav_ssl_verify_cert + "", # calendar_url + "", # calendar_name + "", # features + "", # inherits + "", # time_tracking + "abort", + ] + config = _drive(monkeypatch, inputs, secrets=["hunter2"]) + conn = extract_conn_params_from_section(config["default"]) + assert conn["url"] == "https://calendar.example.com/dav" + assert conn["username"] == "user" + assert conn["password"] == "hunter2" + assert conn["proxy"] == "http://proxy.example.com" + assert "ssl_verify_cert" in conn diff --git a/tests/test_interactive.py b/tests/test_interactive.py new file mode 100644 index 0000000..3b0771c --- /dev/null +++ b/tests/test_interactive.py @@ -0,0 +1,46 @@ +from unittest.mock import MagicMock, patch + +import pytest +from caldav import Todo + +from tests.test_lib import todo + + +def test_interactive_edit_start_without_time_tracking_config(): + """The interactive prompt now advertises `start`, but add_time_tracking + raises NotImplementedError when extra_config.time_tracking is unset - the + default. That must be reported as a message, not an unhandled traceback + that tears down the rest of the check-due session.""" + from plann.interactive import _interactive_edit + + obj = Todo(data=todo) + obj.parent = MagicMock(extra_config={}) + + with patch('plann.interactive.click.prompt', side_effect=['start', 'ignore']): + with patch('plann.interactive.click.echo') as echo: + _interactive_edit(obj) + + printed = " ".join(str(c) for c in echo.call_args_list) + assert 'time_tracking' in printed or 'time tracking' in printed.lower() + + +def test_set_relations_blank_line_does_not_orphan_children(): + """A blank or comment line left behind in the relations editor used to + abort (the old get_obj raised NotImplementedError). _get_obj_from_line + returns None instead, so an unguarded parent silently reaches + _adjust_relations(None, children) - which strips the children's PARENT + relation and saves them. A stray newline must never mutate data.""" + from plann.interactive import _set_relations_from_text_list + + calendar = MagicMock() + calendar.object_by_uid.return_value = MagicMock() + + ## "uidA", then a blank line the user left behind, then an indented child + some_list = ['uidA: task A', ' ', ' uidB: task B'] + + with patch('plann.interactive._adjust_relations') as adjust: + with pytest.raises(NotImplementedError): + _set_relations_from_text_list(calendar, some_list) + + for call in adjust.call_args_list: + assert call.args[0] is not None, "_adjust_relations called with parent=None" diff --git a/tests/test_lib.py b/tests/test_lib.py index 6b4a0a0..f47155a 100644 --- a/tests/test_lib.py +++ b/tests/test_lib.py @@ -6,7 +6,11 @@ from plann.lib import ( _add_category, + _add_comma_list, _adjust_ical_relations, + _caldav_objclass, + _component_type, + _process_set_arg, _procrastinate, _set_something, _split_vcal, @@ -31,7 +35,69 @@ END:VTODO END:VCALENDAR""" -## find_calendars tested in test_functional.py +## find_calendars also tested in test_functional.py + +class TestFindCalendars: + """The heavy lifting (connection parameter extraction, URL resolution + from features, calendar lookup) is delegated to the caldav library - + these tests only verify the plann-side glue.""" + + ## auto-connect hints instead of a caldav_url - the caldav library + ## resolves the URL from these. (A dict rather than a profile name like + ## "ecloud" to keep the test independent of compatibility_hints, and a + ## username without @ to avoid triggering RFC6764 network discovery) + features = {'auto-connect.url': {'domain': 'calendar.example.com', 'scheme': 'https', 'basepath': '/dav'}} + + def _find_calendars(self, args): + class FakeCalendar: + pass + from plann.lib import find_calendars + with patch('caldav.davclient.DAVClient.principal') as principal: + cal = FakeCalendar() + principal.return_value.calendars.return_value = [cal] + principal.return_value.get_calendars.return_value = [cal] + return find_calendars(args, raise_errors=True), cal + + def test_explicit_url(self): + calendars, cal = self._find_calendars({ + 'caldav_url': 'https://calendar.example.com/dav', + 'caldav_user': 'user', 'caldav_pass': 'hunter2'}) + assert calendars == [cal] + + def test_features_without_url(self): + """A config section without caldav_url should work when the URL can + be derived from the features (auto-connect.url hints).""" + calendars, cal = self._find_calendars({ + 'caldav_user': 'user', 'caldav_pass': 'hunter2', + 'features': self.features}) + assert calendars == [cal] + + def test_no_connection_params(self): + calendars, cal = self._find_calendars({}) + assert calendars == [] + + def test_extra_config(self): + """The extra_config section key (used i.a. for the time tracking + integration, cf. add_time_tracking) should be attached to all + calendars found, defaulting to an empty dict.""" + class FakeCalendar: + pass + from plann.lib import find_calendars + with patch('caldav.davclient.DAVClient.principal') as principal: + cal = FakeCalendar() + principal.return_value.calendars.return_value = [cal] + principal.return_value.get_calendars.return_value = [cal] + calendars = find_calendars({ + 'caldav_url': 'https://calendar.example.com/dav', + 'extra_config': {'time_tracking': ['timew']}}, + raise_errors=True) + assert calendars == [cal] + assert cal.extra_config == {'time_tracking': ['timew']} + + calendars = find_calendars( + {'caldav_url': 'https://calendar.example.com/dav'}, + raise_errors=True) + assert calendars[0].extra_config == {} def test_summary(): t = Todo() @@ -43,6 +109,24 @@ def test_summary(): t.icalendar_component.pop('DESCRIPTION') assert(_summary(t) == "19970901T130000Z-123404@host.com") +def test_component_type(): + from caldav import Event, Journal + t = Todo() + t.data = todo + assert _component_type(t) == 'VTODO' + assert _component_type(t.icalendar_component) == 'VTODO' + + ## a description text that mentions BEGIN:VEVENT must not be misclassified + t.icalendar_component['DESCRIPTION'] = 'paste this BEGIN:VEVENT into the calendar' + assert _component_type(t) == 'VTODO' + + assert _caldav_objclass(todo) is Todo + assert _caldav_objclass(t.data) is Todo + event_ical = todo.replace('VTODO', 'VEVENT').replace('DUE:19970416T045959Z\n', '') + assert _caldav_objclass(event_ical) is Event + journal_ical = todo.replace('VTODO', 'VJOURNAL').replace('DUE:19970416T045959Z\n', '') + assert _caldav_objclass(journal_ical) is Journal + @pytest.mark.parametrize("method", [add_time_tracking_timew, add_time_tracking]) @patch("plann.lib.subprocess.run") def test_add_time_tracking_timew(mock_run, method): @@ -51,7 +135,9 @@ def test_add_time_tracking_timew(mock_run, method): obj = Todo() obj.data = todo obj.parent=Calendar() - obj.parent.extra_config={'time_tracking': ['timew']} + ## "timewarrior" as in the config file documentation - "timew" and + ## "Timewarrior" should also be accepted + obj.parent.extra_config={'time_tracking': ['timewarrior']} method(obj, ts1, ts2) @@ -60,17 +146,88 @@ def test_add_time_tracking_timew(mock_run, method): assert cmd_arr[0:5] == ['timew', 'track', '2020-02-20T20:02', '-', '2020-02-20T20:20'] +def test_list_separator(): + """_list joins output with newlines by default, but the separator + should be configurable.""" + from plann.lib import _list + with patch('plann.lib.click.echo_via_pager') as pager: + _list(['a', 'b', 'c']) + pager.assert_called_once_with('a\nb\nc') + with patch('plann.lib.click.echo_via_pager') as pager: + _list(['a', 'b', 'c'], separator=' | ') + pager.assert_called_once_with('a | b | c') + + +def test_interactive_edit_start(): + """The interactive 'start' command kicks off time tracking and then + re-prompts, so a follow-up command can be given for the same task.""" + from plann.interactive import _interactive_edit + obj = Todo() + obj.data = todo + prompts = iter(['start', 'ignore']) + with patch('plann.interactive.add_time_tracking') as att: + with patch.object(obj, 'save'): + with patch('click.echo'): + with patch('click.prompt', side_effect=lambda *a, **k: next(prompts)) as prompt: + _interactive_edit(obj) + att.assert_called_once_with(obj) + assert prompt.call_count == 2 + + +def _comma_list_set(obj, prop): + """Read a comma-list property (CATEGORIES single-line vCategory, or + RESOURCES multi-line list) back as a set of strings.""" + val = obj.icalendar_component.get(prop) + if val is None: + return set() + if hasattr(val, 'cats'): + return {str(x) for x in val.cats} + if isinstance(val, list): + return {str(x) for x in val} + return {str(val)} + + def test_add_set_category(): t = Todo() t.data = todo _add_category(t, 'foo') assert 'CATEGORIES:foo' in t.data + assert _comma_list_set(t, 'CATEGORIES') == {'foo'} _add_category(t, 'bar') - set(t.icalendar_component['CATEGORIES'].cats) == {'foo', 'bar'} + assert _comma_list_set(t, 'CATEGORIES') == {'foo', 'bar'} + ## singular "category" appends ... _set_something(t, 'category', 'zoo') - set(t.icalendar_component['CATEGORIES'].cats) == {'foo', 'bar', 'zoo'} + assert _comma_list_set(t, 'CATEGORIES') == {'foo', 'bar', 'zoo'} + ## ... while plural "categories" replaces (and splits on comma) _set_something(t, 'categories', 'zoo,bar') - set(t.icalendar_component['CATEGORIES'].cats) == {'bar', 'zoo'} + assert _comma_list_set(t, 'CATEGORIES') == {'zoo', 'bar'} + + +def test_add_set_resources(): + """RESOURCES is the other RFC 5545 comma-list property and should behave + like CATEGORIES: append via _add_comma_list, replace via plural set.""" + t = Todo() + t.data = todo + _add_comma_list(t, 'resources', ['Projector']) + _add_comma_list(t, 'resources', ['Easel', 'Screen']) + assert _comma_list_set(t, 'RESOURCES') == {'Projector', 'Easel', 'Screen'} + ## plural "resources" replaces + _set_something(t, 'resources', ['Whiteboard']) + assert _comma_list_set(t, 'RESOURCES') == {'Whiteboard'} + + +def test_process_set_arg_comma_list(): + """Plural forms split a lone comma value; singular forms keep it literal. + Resources must get the same treatment as categories (previously it did not + split).""" + ## plural: comma-split + assert _process_set_arg('categories', ('a,b',), keep_category=True) == {'categories': ['a', 'b']} + assert _process_set_arg('resources', ('a,b',), keep_category=True) == {'resources': ['a', 'b']} + ## singular: comma kept literal (one token) + assert _process_set_arg('category', ('a,b',), keep_category=True) == {'category': ['a,b']} + ## without keep_category, the singular alias is canonicalised to plural + ## (replace semantics, used by the create path -> save_todo(categories=...)) + assert _process_set_arg('category', ('a,b',), keep_category=False) == {'categories': ['a,b']} ## _hasreltype is skipped as for now (too small and only used in _procrastinate) @@ -131,8 +288,47 @@ def test_adjust_ical_relations(): assert(rels['PARENT'] == {'PARENT-A0', 'PARENT-A2', 'PARENT-B0', 'PARENT-B2'}) assert(rels['CHILD'] == {'CHILD-A0', 'CHILD-A1', 'CHILD-A2'}) -#def test_split_vcals(): -## TODO +def _one_vcal(uid): + return ( + "BEGIN:VCALENDAR\n" + "VERSION:2.0\n" + "PRODID:-//Example Corp.//CalDAV Client//EN\n" + "BEGIN:VEVENT\n" + f"UID:{uid}\n" + f"SUMMARY:event {uid}\n" + "DTSTART:20250101T100000Z\n" + "END:VEVENT\n" + "END:VCALENDAR" + ) + + +def test_split_vcals(): + """Multiple concatenated VCALENDAR streams are split into one entry each.""" + from icalendar import Calendar + + from plann.lib import _split_vcals + + joined = _one_vcal("a") + "\n" + _one_vcal("b") + "\n" + _one_vcal("c") + output = _split_vcals(joined) + assert len(output) == 3 + ## each piece must round-trip as a standalone single-event VCALENDAR + uids = [] + for piece in output: + cal = Calendar.from_ical(piece) + events = [c for c in cal.subcomponents if c.name == 'VEVENT'] + assert len(events) == 1 + uids.append(str(events[0]['UID'])) + assert uids == ['a', 'b', 'c'] + + +def test_split_vcals_crlf(): + """CRLF line endings (RFC 5545 canonical) must split too - the previous + hand-rolled LF-only scanner silently returned nothing (code review C11).""" + from plann.lib import _split_vcals + + joined = (_one_vcal("a") + "\n" + _one_vcal("b")).replace("\n", "\r\n") + assert len(_split_vcals(joined)) == 2 + def test_split_vcal(): ## This VCALENDAR contains three events, but only two separate @@ -189,3 +385,93 @@ def test_split_vcal(): """ output = _split_vcal(input) assert(len(output) == 2) + + +def test_split_vcal_yields_ical_strings(): + """`add ical` feeds every element of _split_vcal() straight into + _caldav_objclass(), which parses text - so _split_vcal must yield ical + strings, exactly as _split_vcals() does for the multi-VCALENDAR case. + + Yielding icalendar.Calendar objects instead made the ordinary + single-VCALENDAR `plann add ical` abort with + `ValueError: Expected StringType with content lines`.""" + parts = list(_split_vcal(todo)) + assert len(parts) == 1 + for part in parts: + assert isinstance(part, str), f"expected ical text, got {type(part).__name__}" + ## and the component type must survive the round-trip + assert _caldav_objclass(part) is Todo + + +class _FakeCalendar: + """A minimal in-memory calendar that counts get_object_by_uid lookups, so + tests can assert how many server round-trips a traversal would issue.""" + def __init__(self): + import collections + self.objs = {} + self.fetch_counts = collections.Counter() + self.url = "http://cal.example/" + + def add(self, todo): + todo.parent = self + self.objs[str(todo.icalendar_component['UID'])] = todo + + def get_object_by_uid(self, uid): + import caldav + uid = str(uid) + self.fetch_counts[uid] += 1 + if uid not in self.objs: + raise caldav.error.NotFoundError(uid) + return self.objs[uid] + + def get_display_name(self): + return "Fake" + + +def _todo_with_rels(uid, rels): + """Build a Todo with the given (reltype, target_uid) RELATED-TO links.""" + rel_lines = "".join(f"RELATED-TO;RELTYPE={rt}:{target}\n" for rt, target in rels) + obj = Todo() + obj.data = ( + "BEGIN:VCALENDAR\nVERSION:2.0\nPRODID:-//Example Corp.//CalDAV Client//EN\n" + "BEGIN:VTODO\n" + f"UID:{uid}\n" + f"SUMMARY:task {uid}\n" + f"{rel_lines}" + "END:VTODO\nEND:VCALENDAR" + ) + return obj + + +def test_list_top_down_caches_relative_fetches(): + """A hierarchical `list --top-down` must not re-fetch the same task from + the server once per graph edge - each related object is fetched at most + once per traversal (code review E3).""" + from plann.lib import _list + + cal = _FakeCalendar() + ## parent p with two children c1, c2 (bidirectionally linked) + p = _todo_with_rels('p', [('CHILD', 'c1'), ('CHILD', 'c2')]) + c1 = _todo_with_rels('c1', [('PARENT', 'p')]) + c2 = _todo_with_rels('c2', [('PARENT', 'p')]) + for obj in (p, c1, c2): + cal.add(obj) + + with patch('plann.lib.click.echo_via_pager'): + _list([c1, c2], top_down=True) + + ## without caching, p is fetched once per child (and more via recursion) + assert cal.fetch_counts['p'] == 1, dict(cal.fetch_counts) + assert all(count <= 1 for count in cal.fetch_counts.values()), dict(cal.fetch_counts) + + +def test_procrastinate_has_no_breakpoint(): + """A stray breakpoint() drops the user into pdb (or appears to hang when + there is no tty). Code review item #13 - the recursive postpone-parent + path in _procrastinate had one guarded by an inspect.stack() depth check, + which is reachable from `interactive check-due` / `dismiss-panic`.""" + import inspect as _inspect + + source = _inspect.getsource(_procrastinate) + assert 'breakpoint()' not in source + assert 'inspect.stack()' not in source diff --git a/tests/test_plann_cli.py b/tests/test_plann_cli.py index 76d8953..2798e84 100644 --- a/tests/test_plann_cli.py +++ b/tests/test_plann_cli.py @@ -1,4 +1,303 @@ ## Check https://click.palletsprojects.com/en/8.1.x/testing/ -## TODO! add some tests +from unittest.mock import patch +import caldav +from caldav import Todo +from click.testing import CliRunner + +import plann.cli as cli_mod +import plann.commands as commands_mod +from plann.cli import _LazyCalendars, cli, delete, edit +from plann.commands import _process_add_args, _set_task_attribs, _sort_key_function, _todos_missing + + +def _make_todo(uid, *, category=False, due=False, priority=False, dtstart=False): + lines = ["UID:" + uid, "SUMMARY:task " + uid, "STATUS:NEEDS-ACTION"] + if category: + lines.append("CATEGORIES:work") + if dtstart: + lines.append("DTSTART:20250101T100000Z") + if due: + lines.append("DUE:20250102T100000Z") + if priority: + lines.append("PRIORITY:5") + obj = Todo() + obj.data = ( + "BEGIN:VCALENDAR\nVERSION:2.0\nPRODID:-//Example Corp.//CalDAV Client//EN\n" + "BEGIN:VTODO\n" + "\n".join(lines) + "\nEND:VTODO\nEND:VCALENDAR" + ) + return obj + + +class _FakeCtx: + def __init__(self): + self.obj = {'calendars': [], 'objs': []} + + +def test_todos_missing(): + """_todos_missing keeps only the tasks where the property is absent.""" + with_cat = _make_todo('a', category=True) + without_cat = _make_todo('b') + result = _todos_missing([with_cat, without_cat], 'categories') + assert [str(o.icalendar_component['UID']) for o in result] == ['b'] + + +def test_set_task_attribs_fetches_once(): + """set-task-attribs must fetch the task list once and filter client-side + per attribute, not issue a fresh server select per attribute (code review + E4).""" + ## fully-specified todos => nothing is missing => no interactive prompting + todos = [_make_todo(uid, category=True, due=True, priority=True, dtstart=True) + for uid in ('a', 'b', 'c')] + calls = [] + + def fake_select(ctx, **kwargs): + calls.append(kwargs) + ctx.obj['objs'] = list(todos) + + ctx = _FakeCtx() + with patch.object(commands_mod, '_select', fake_select): + _set_task_attribs(ctx) + assert len(calls) == 1, f"expected a single fetch, got {len(calls)}" + + +def test_set_task_attribs_prompts_missing_attribute(): + """A task missing only a category is prompted for it (and only it), and + the entered category is saved - the single-fetch refactor still drives the + interactive flow.""" + ## has due/priority/dtstart, lacks only a category + todo = _make_todo('a', due=True, priority=True, dtstart=True) + + def fake_select(ctx, **kwargs): + ctx.obj['objs'] = [todo] + + ctx = _FakeCtx() + with patch.object(commands_mod, '_select', fake_select), \ + patch.object(todo, 'save') as save, \ + patch('plann.commands.click.echo'), \ + patch('plann.commands.click.prompt', return_value='work') as prompt: + _set_task_attribs(ctx) + + ## exactly one prompt (the missing category), and it got persisted + assert prompt.call_count == 1 + save.assert_called_once() + assert 'work' in todo.data + +_TODO = """BEGIN:VCALENDAR +VERSION:2.0 +PRODID:-//Example Corp.//CalDAV Client//EN +BEGIN:VTODO +UID:{uid} +DTSTAMP:19970901T130000Z +SUMMARY:task {uid} +PRIORITY:{priority} +END:VTODO +END:VCALENDAR""" + + +def _todo(uid, priority): + obj = Todo() + obj.data = _TODO.format(uid=uid, priority=priority) + return obj + + +def test_sort_key_function_priority(): + """A bare property name sorts on that icalendar property.""" + reverse, fkey = _sort_key_function('PRIORITY') + assert reverse is False + objs = [_todo('a', 3), _todo('b', 1), _todo('c', 2)] + objs.sort(key=fkey) + assert [str(o.icalendar_component['UID']) for o in objs] == ['b', 'c', 'a'] + + +def test_sort_key_function_reverse(): + """A leading '-' reverses the sort.""" + reverse, fkey = _sort_key_function('-PRIORITY') + assert reverse is True + objs = [_todo('a', 3), _todo('b', 1), _todo('c', 2)] + objs.sort(key=fkey, reverse=reverse) + assert [str(o.icalendar_component['UID']) for o in objs] == ['a', 'c', 'b'] + + +def test_sort_key_function_compiles_template_once(): + """A template sort key must be compiled once, not rebuilt on every + comparison during the sort (code review E2).""" + objs = [_todo(uid, p) for uid, p in (('a', 3), ('b', 1), ('c', 2), ('d', 5), ('e', 4))] + with patch('plann.commands.Template', wraps=commands_mod.Template) as template_cls: + reverse, fkey = _sort_key_function('{PRIORITY}') + objs.sort(key=fkey, reverse=reverse) + assert template_cls.call_count == 1 + assert [str(o.icalendar_component['UID']) for o in objs] == ['b', 'c', 'a', 'e', 'd'] + + +def test_configure_command_registered(): + """The interactive configuration is wired into the click CLI as + `plann configure` (code review C8 re-wiring).""" + assert 'configure' in cli.commands + + +def test_lazy_calendars_resolves_once(): + """_LazyCalendars defers discovery until first use, then caches it.""" + calls = [] + + def discover(): + calls.append(1) + return ['a', 'b'] + + lazy = _LazyCalendars(discover) + assert calls == [] ## not resolved just by constructing + assert len(lazy) == 2 ## first access triggers discovery + assert list(lazy) == ['a', 'b'] + assert lazy[0] == 'a' + assert bool(lazy) is True + assert calls == [1] ## resolved exactly once + + +def test_calendar_discovery_deferred_for_help(): + """Showing subcommand --help must not trigger calendar discovery (which + talks to the network), even with a caldav_url given (code review E1).""" + calls = [] + orig = cli_mod.find_calendars + + def spy(*a, **k): + calls.append(1) + return orig(*a, **k) + + cli_mod.find_calendars = spy + try: + res = CliRunner().invoke( + cli, ['--skip-config', '--caldav-url', 'http://example.invalid/', 'select', '--help']) + assert res.exit_code == 0, res.output + assert calls == [], "discovery should be deferred when only showing help" + finally: + cli_mod.find_calendars = orig + + +def _option_names(cmd): + names = set() + for param in cmd.params: + names.update(param.opts) + return names + + +def _find_option(cmd, name): + for param in cmd.params: + if name in param.opts: + return param + return None + + +def test_edit_exposes_comma_list_options(): + """categories and resources are both exposed in singular + plural for both + add (append) and the existing set (replace) verbs - so the user does not + have to remember which form to use.""" + names = _option_names(edit) + for opt in ( + '--add-category', + '--add-categories', + '--add-resource', + '--add-resources', + '--set-category', + '--set-categories', + '--set-resources', + ): + assert opt in names, f"missing {opt}" + + +def test_no_set_resource_singular(): + """There is deliberately no --set-resource (singular replace).""" + assert '--set-resource' not in _option_names(edit) + + +def test_set_category_marked_deprecated(): + """--set-category keeps working (it appends) but its help flags it as + deprecated in favour of --add-category / --set-categories.""" + opt = _find_option(edit, '--set-category') + assert opt is not None + assert 'deprecat' in (opt.help or '').lower() + + +def test_process_add_args(): + """--add-* options are collected as (canonical_property, tokens) to append: + plural splits on comma, singular keeps it literal.""" + kwargs = { + 'add_category': ('a,b',), + 'add_categories': ('x,y',), + 'add_resource': ('R1,R2',), + 'add_resources': ('S1,S2',), + 'set_summary': 'unrelated', + } + result = _process_add_args(kwargs) + assert ('categories', ['a,b']) in result + assert ('categories', ['x', 'y']) in result + assert ('resources', ['R1,R2']) in result + assert ('resources', ['S1', 'S2']) in result + ## add_* keys are consumed; unrelated keys are left untouched + assert not any(k.startswith('add_') for k in kwargs) + assert 'set_summary' in kwargs + + +class _FakeCalNotFound: + """A calendar that never has the requested uid.""" + def get_object_by_uid(self, uid): + raise caldav.error.NotFoundError(uid) + + +def test_select_warns_on_missing_uid(): + """By default, a --uid that matches nothing in any calendar produces a + warning naming the missing uid (https://github.com/tobixen/plann/issues/42).""" + ctx = _FakeCtx() + ctx.obj['calendars'] = [_FakeCalNotFound()] + with patch('plann.commands.click.echo') as echo: + commands_mod._select(ctx, uid=('asdf',)) + assert any('asdf' in str(c.args[0]) for c in echo.call_args_list), \ + "expected a warning naming the missing uid" + ## warnings go to stderr so they do not pollute scriptable stdout + assert all(c.kwargs.get('err') for c in echo.call_args_list) + + +def test_select_no_warn_on_missing_uid(): + """--no-warn-on-missing-uid restores the old silent behaviour.""" + ctx = _FakeCtx() + ctx.obj['calendars'] = [_FakeCalNotFound()] + with patch('plann.commands.click.echo') as echo: + commands_mod._select(ctx, uid=('asdf',), warn_on_missing_uid=False) + assert echo.call_count == 0 + + +def test_select_no_warn_when_uid_found(): + """A uid that is found produces no missing-uid warning.""" + todo = _make_todo('a') + + class _FakeCalFound: + def get_object_by_uid(self, uid): + return todo + + ctx = _FakeCtx() + ctx.obj['calendars'] = [_FakeCalFound()] + with patch('plann.commands.click.echo') as echo: + commands_mod._select(ctx, uid=('a',)) + assert echo.call_count == 0 + assert ctx.obj['objs'] == [todo] + + +def test_delete_reports_deleted_item(): + """delete must tell the user what it did - deleting a selected item should + name it and actually call .delete() + (https://github.com/tobixen/plann/issues/42).""" + todo = _make_todo('a') + with patch.object(todo, 'delete') as deleted: + res = CliRunner().invoke(delete, obj={'objs': [todo]}) + assert res.exit_code == 0, res.output + deleted.assert_called_once() + assert 'task a' in res.output + + +def test_delete_reports_empty_selection(): + """delete on an empty selection (e.g. --uid matched nothing) must say so + rather than silently producing no output + (https://github.com/tobixen/plann/issues/42).""" + res = CliRunner().invoke(delete, obj={'objs': []}) + assert res.exit_code == 0, res.output + assert res.output.strip(), "expected some feedback on an empty selection" diff --git a/tests/test_timespec.py b/tests/test_timespec.py index 2c3fda7..1241946 100644 --- a/tests/test_timespec.py +++ b/tests/test_timespec.py @@ -3,6 +3,7 @@ import pytest from plann.lib import _ensure_ts, parse_add_dur, parse_dt, parse_timespec, tz +from plann.timespec import DURATION_RE, is_duration utc = timezone.utc @@ -47,7 +48,18 @@ def testParseDt(self, input): (datetime(2020,2,20),'4d', datetime(2020,2,24)), (datetime(2020,2,20),'1w1s', datetime(2020,2,27,0,0,1)), (datetime(2020,2,20),'2y1d', datetime(2022,2,21)), - (None, '1s', timedelta(seconds=1)) + (None, '1s', timedelta(seconds=1)), + ## Regression tests for the code review bugs #6, #7 and #8. + ## #6: the year constant was 1314000 seconds (~15 days), not 31536000. + (None, '1y', timedelta(days=365)), + ## #8: `diff` was reassigned rather than accumulated, so only the + ## last unit of a compound duration survived. + (None, '1h30m', timedelta(minutes=90)), + (None, '2d3h', timedelta(days=2, hours=3)), + ## #7: the year branch used to do datetime arithmetic that blew up on + ## a plain date, and on a Feb 29 base date. + (date(2021,1,8), '1y', date(2022,1,8)), + (date(2020,2,29), '1y', date(2021,2,28)), ]) def test_parseAddDur(self, dt, dur, expected): if isinstance(dt, datetime): @@ -188,11 +200,49 @@ def test_day_name(self): result = parse_dt("Monday") assert result.weekday() == 0 # Monday + def test_relative_future(self): + tz.implicit_timezone = "Europe/Oslo" + result = parse_dt("in 2 days") + expected = (datetime.now().astimezone() + timedelta(days=2)).date() + got = result.date() if isinstance(result, datetime) else result + assert got == expected + + def test_yesterday_via_timespec(self): + """Natural-language dates should also flow through parse_timespec().""" + tz.implicit_timezone = "Europe/Oslo" + start, end = parse_timespec("yesterday") + expected = (datetime.now().astimezone() - timedelta(days=1)).date() + got = start.date() if isinstance(start, datetime) else start + assert got == expected + assert end is None + def test_invalid_raises(self): with pytest.raises(ValueError): parse_dt("not a date at all !!!!") +class TestDurationGrammar: + """The [smhdwy] relative-duration grammar is centralised in timespec.py.""" + + @pytest.mark.parametrize("text,expected", [ + ("1y1w2h", True), + ("2.5h", True), + ("30m", True), + ("1s", True), + ("yesterday", False), + ("2021-01-08", False), + ("3M", False), # months not part of the grammar (yet) + ("", False), + ]) + def test_is_duration(self, text, expected): + assert is_duration(text) == expected + + def test_duration_re_is_shared(self): + """The same compiled regex backs is_duration() and the suffix matcher.""" + assert DURATION_RE.fullmatch("1y1w2h") + assert not DURATION_RE.fullmatch("1y1w2x") + + def test_ensure_ts(): now = datetime.now() utcnow = now.astimezone(utc)