Skip to content

Add reminders and location reminders CRUD support#234

Open
scottlovegrove wants to merge 3 commits intomainfrom
scottl/reminders
Open

Add reminders and location reminders CRUD support#234
scottlovegrove wants to merge 3 commits intomainfrom
scottl/reminders

Conversation

@scottlovegrove
Copy link

Summary

  • Add Reminder and LocationReminder models with appropriate field aliases (item_idtask_id)
  • Add full CRUD methods for both regular reminders (/reminders) and location reminders (/location_reminders) in sync and async APIs
  • Regular reminders support relative (via minute_offset) and absolute (via due) types
  • Location reminders support geofenced triggers (on_enter/on_leave) with configurable radius

Test plan

  • All 10 new tests pass (5 per reminder type: get, get_all, add, update, delete)
  • All 88 tests pass (78 existing + 10 new)
  • ruff format clean
  • No new lint errors (3 pre-existing in models.py)

🤖 Generated with Claude Code

Add full CRUD operations for both regular reminders (relative/absolute)
and location reminders, including sync and async API methods, models,
and tests.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@scottlovegrove scottlovegrove requested a review from a team as a code owner March 24, 2026 11:43
@scottlovegrove scottlovegrove requested review from lukemerrett and removed request for a team March 24, 2026 11:43
@scottlovegrove scottlovegrove self-assigned this Mar 24, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link

@doistbot-app doistbot-app bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR introduces valuable CRUD support for both regular and location reminders across the sync and async APIs. The implementation is solid, and the comprehensive test coverage is much appreciated. There are a few areas for refinement, specifically around adding the missing is_urgent field to the Reminder model, flattening input parameters rather than reusing the read-only Due response model, and ensuring test fixtures use distinct values to properly verify the item_id to task_id aliasing.

Share FeedbackReview Logs

- Replace Due model input param with flat fields (due_string, due_date,
  due_datetime, due_lang, due_timezone) matching add_task pattern
- Use distinct id/item_id values in test fixtures to properly exercise
  the item_id -> task_id alias

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@lukemerrett
Copy link

@doistbot Please re-review this PR

Copy link

@lukemerrett lukemerrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, have asked DoistBot to re-review it

@doistbot-app
Copy link

doistbot-app bot commented Mar 25, 2026

Hi @scottlovegrove, thanks for putting this PR together to support reminders and location reminders! The overall structure and tests look solid, but I noticed a couple of potential payload issues in api.py and api_async.py that we should verify against the API behavior.

First, in add_reminder, the request payload is built using kwargs_without_none(..., reminder_type=reminder_type, ...). Since the Reminder response model maps this property as type, it's highly likely the Todoist REST API expects the key "type" in the create request rather than "reminder_type". You can easily fix this by passing type=reminder_type to kwargs_without_none, since type is perfectly valid as a Python keyword argument.

Second, update_reminder currently doesn't accept a reminder_type parameter. If the API requires the type property when a user updates a reminder to switch it from relative to absolute (or vice versa), users won't be able to do so with the current signature. Additionally, since add_reminder defaults to "relative", users might accidentally misconfigure their request if they pass a due_date but forget to explicitly set reminder_type="absolute". It might be worth inferring the type dynamically based on the presence of due_* fields, or simply removing the default to require explicit intent.

Let me know what you think!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants