feat(client): return created events from insert_event(s)#103
feat(client): return created events from insert_event(s)#103TimeToBuildBob wants to merge 1 commit intoActivityWatch:masterfrom
Conversation
The server already returns created events (with assigned IDs) in the response body, but the client was discarding them. Now insert_event returns Optional[Event] and insert_events returns Optional[List[Event]]. This is backward-compatible: callers that ignore the return value still work. The main use case is getting server-assigned event IDs for later update/delete operations. Fixes ActivityWatch#77
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 0513857 in 6 seconds. Click for details.
- Reviewed
30lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_naKw15lRQ5iOgqaO
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Greptile SummaryModified Key changes:
The implementation follows existing code patterns in the codebase and maintains consistency with error handling approaches used in other methods. Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 0513857 |
|
Hmm, please inspect the log/blame here. I think we removed the "return event" since in the aw-server-rust implementation events are received and their IDs are difficult to predict in time. Something like it gets sent to a queue or something, lacking an efficient way to return the event with set ID. (iirc) |
|
Investigated the git log/blame as requested. Here's what I found: Timeline:
Key finding: The removal was not due to aw-server-rust's queue architecture making IDs unpredictable. The aw-server-rust worker queue is synchronous ( Server-side behavior today:
Options:
Given that aw-server-rust is the recommended server and the Python server's response format is inconsistent anyway, option 1 seems most pragmatic. But if you'd prefer I close this, happy to do so. |
|
Then I think we should fix the Python server's inconsistent response format in a separate PR so that we actually target both once everything lands.
I guess fix it now? |
…-rust)
The POST /api/0/buckets/{id}/events endpoint previously returned either a
single event dict or null, inconsistent with aw-server-rust which always
returns a list of events with server-assigned IDs.
Changes:
- api.py: create_events() now returns List[Event], using insert_one for
single events (to get ID) and insert_many for bulk
- rest.py: Always serialize response as a list of event dicts
- tests: Add test_insert_event_returns_list and test_insert_events_returns_list
This enables aw-client to use response.json()[0] uniformly against both
server implementations (see ActivityWatch/aw-client#103).
|
Submitted ActivityWatch/aw-server#158 to fix the Python server's response format — |
|
See the discussion about insert_event vs insert_events (one returns ID, the other does not): ActivityWatch/aw-server#158 |
Summary
insert_event()now returnsOptional[Event](wasNone)insert_events()now returnsOptional[List[Event]](wasNone)This is backward-compatible — callers that ignore the return value still work fine. The main use case is getting server-assigned event IDs for later update/delete operations.
Fixes #77.
Test plan
Important
insert_eventandinsert_eventsinaw_client/client.pynow return created events, allowing retrieval of server-assigned IDs.insert_event()now returnsOptional[Event]instead ofNone.insert_events()now returnsOptional[List[Event]]instead ofNone.This description was created by
for 0513857. You can customize this summary. It will automatically update as commits are pushed.