Skip to content

Add event/calendar CRUD operations and recurrence support#1

Open
jorgemitchell0808 wants to merge 3 commits intoschappim:mainfrom
jorgemitchell0808:release/1.3.0
Open

Add event/calendar CRUD operations and recurrence support#1
jorgemitchell0808 wants to merge 3 commits intoschappim:mainfrom
jorgemitchell0808:release/1.3.0

Conversation

@jorgemitchell0808
Copy link

  • Add update event command with all event fields (single events only)
  • Add calendar create/update/delete commands
  • Add recurrence rules (daily/weekly/monthly with advanced options)
  • Add travel time, alarms, URL, and availability fields
  • Add geocoding for structured location resolution
  • Restructure README with clear hierarchy and command reference
  • Bump version to 1.3.0

Copy link
Owner

@schappim schappim left a comment

Choose a reason for hiding this comment

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

Hey Jorge, thanks for taking the time to put this together — this is a really solid first contribution and I appreciate the ambition here. Calendar CRUD, recurrence rules, geocoding, travel time — you've clearly thought about what would make ekctl more useful. The overall structure and approach is great.

I've gone through the diff in detail and have some feedback before we can get this merged. Nothing insurmountable — mostly cleanup and a couple of bugs.

Bugs to fix

Duplicate travel time setting in updateEvent()
The travel time is being set twice with event.setValue(tTime, forKey: "travelTime") — looks like a copy-paste leftover. Just remove the duplicate block.

Alarms sign issue
The --alarms flag description says "minutes relative to start", but the code does $0 * -60. If a user passes 10,60 (meaning 10 and 60 minutes before), that works. But if they pass -30 (thinking negative = before), the double negation produces a positive offset — meaning 30 minutes after the event. Either enforce positive-only input and document it clearly, or handle the sign more carefully.

Silent failure in UpdateEvent
When date parsing fails in UpdateEvent, it throws ExitCode.failure without printing a JSON error message. Every other command prints a proper JSONOutput.error(...) before throwing — this one should too, so callers can programmatically understand what went wrong.

Code quality

Inconsistent geocoding
addEvent uses the shared resolveLocation() helper (5s timeout), but updateEvent has its own inline geocoding with a 2s timeout and slightly different logic. Let's use resolveLocation() in both places to keep things consistent.

Private API for travel time
event.setValue(tTime, forKey: "travelTime") is using KVC on a private property — it works today but could break with future macOS updates. Worth adding a comment acknowledging this, so future maintainers know it's intentional.

Dead parameters
travelStartLocation and transportType exist in the updateEvent and addEvent function signatures but aren't wired up to any CLI flags or used anywhere. Let's remove them for now — we can add them properly when they're ready.

Truncated MARK comment
Line has // MARK: - C — should be // MARK: - Calendar Commands or similar.

Priority type change
Changing AddReminder.priority from Int? to String? is a subtle breaking change. The fallback silently defaults invalid strings to 0 instead of reporting an error. If someone had a script passing --priority high, it would silently become "no priority" instead of failing.

README

The restructuring looks cleaner overall, but I'd like to keep some content that was removed:

  • The date format reference table — users reference that a lot
  • The scripting examples section (get calendar by name, list today's events, export to CSV) — that's some of the most practical documentation we have
  • The alias list JSON output example

Also, missing newline at end of file.

Nice to have (not blocking)

  • Tests for the new commands would be great, even basic ones
  • An update reminder command would round out the CRUD symmetry, but fine to add in a follow-up

Really appreciate you tackling this — the feature set is exactly what I've been wanting to add. Once these issues are addressed, I'm keen to get this merged. Let me know if any of the feedback is unclear or if you want to discuss a different approach for any of it.

@jorgemitchell0808
Copy link
Author

Hey Marcus, thanks for the detailed review, really really helpful feedback :)

I've pushed a new commit addressing everything:

Bugs fixed

  • Removed the duplicate event.setValue(tTime, forKey: "travelTime") in updateEvent()
  • Fixed the alarms sign issue — positive numbers now mean minutes before, + prefix means after, and the README documents this clearly
  • UpdateEvent now prints a proper JSONOutput.error(...) before throwing on date parse failure

Code quality

  • updateEvent now uses the shared resolveLocation() helper instead of inline geocoding
  • Added a comment on the KVC travel time property acknowledging it's intentional and may break in future macOS updates
  • Removed travelStartLocation and transportType dead parameters
  • Fixed the truncated // MARK: - C comment
  • AddReminder.priority now properly validates the input and returns a JSON error instead of silently defaulting to 0

README

  • Restored the date format reference table
  • Restored the scripting examples section
  • Restored the alias list JSON output example
  • Fixed missing newline at end of file

Nice to haves

  • Added update reminder command included inside the updated README
  • Added tests covering the new commands — ConfigManager alias CRUD, alarm parsing, hex color, date validation, travel time conversion, recurrence interval fallback, priority parsing, and update reminder logic. Also split into a proper ekctlCore library target so tests import ekctlCore directly instead of copying logic into the test file.

Let me know if anything needs a second look!

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