[Feature] added get_history function #91
Conversation
added get_history function
WalkthroughAdds a new function get_history(list_type=None, id=None, start_at=None, end_at=None) in trakt/sync.py to fetch user watch history for movies or episodes, validate list_type, build a sync/history URI with optional filters (type, id, time range), request data, and yield Movie or TVShow objects based on response entries. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant SyncAPI as sync.get_history()
participant Trakt as Trakt API
Client->>SyncAPI: get_history(list_type, id, start_at, end_at)
Note over SyncAPI: Validate list_type ('movies'|'episodes')<br/>Build URI: sync/history[/type][/id][?start_at][&end_at]
SyncAPI->>Trakt: GET sync/history... with query params
Trakt-->>SyncAPI: JSON history entries
alt entry contains "movie"
SyncAPI->>SyncAPI: Construct Movie object
else entry contains "show"
SyncAPI->>SyncAPI: Construct TVShow object
end
SyncAPI-->>Client: List[Movie|TVShow]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
trakt/sync.py (3)
533-533: Avoid shadowing built-in nameidShadowing
idreduces clarity and can confuse tooling. Preferitem_id.Apply this diff:
-def get_history(list_type=None, id=None, start_at=None, end_at=None): +def get_history(list_type=None, item_id=None, start_at=None, end_at=None): @@ - if id: - uri += '/{}'.format(id) + if item_id: + uri += '/{}'.format(item_id)Also applies to: 551-553
16-21: Export the new API in__all__Without adding
get_historyto__all__, star-importers or docs relying on it may miss the new function.Apply this diff:
- 'remove_from_collection', 'search', 'search_by_id', 'checkin_media', - 'delete_checkin'] + 'remove_from_collection', 'search', 'search_by_id', 'checkin_media', + 'delete_checkin', 'get_history']
533-578: Optional: validate date range earlyPrevent surprising server calls by validating
start_at <= end_at(when both provided) before converting to strings.I can add a small helper to normalize inputs to
datetimeand compare them, or write unit tests covering:
- movies vs episodes mapping,
- with and without
list_type,start_at/end_atpermutations and ordering,- presence of
id.
Would you like me to draft these?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
trakt/sync.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
trakt/sync.py (4)
trakt/core.py (1)
get(144-147)trakt/utils.py (1)
timestamp(47-50)trakt/movies.py (1)
Movie(93-387)trakt/tv.py (1)
TVShow(206-570)
🔇 Additional comments (1)
trakt/sync.py (1)
549-553: Use query parameters fortypeandid
The Sync: Get history endpoint acceptstypeandidas query parameters only—do not append/typeor/idin the path (trakt/sync.py, lines 549–553). Remove those concatenations and includetypeandidin?type=…&id=….Likely an incorrect or invalid review comment.
| if not isinstance(start_at, str): | ||
| start_at = timestamp(start_at) | ||
| if start_at: | ||
| uri += '?start_at={start_at}'.format(start_at=start_at) | ||
|
|
||
| if not isinstance(end_at, str): | ||
| end_at = timestamp(end_at) | ||
| if end_at: | ||
| uri += '?end_at={end_at}'.format(end_at=end_at) | ||
|
|
There was a problem hiding this comment.
Fix None handling and join query params correctly for start_at/end_at
timestamp(None) will raise, and using ? twice yields an invalid URL when both params are present.
Apply this diff:
- if not isinstance(start_at, str):
- start_at = timestamp(start_at)
- if start_at:
- uri += '?start_at={start_at}'.format(start_at=start_at)
-
- if not isinstance(end_at, str):
- end_at = timestamp(end_at)
- if end_at:
- uri += '?end_at={end_at}'.format(end_at=end_at)
+ # Normalize timestamps only when provided; then build query string once.
+ if start_at is not None and not isinstance(start_at, str):
+ start_at = timestamp(start_at)
+ if end_at is not None and not isinstance(end_at, str):
+ end_at = timestamp(end_at)
+
+ params = []
+ if start_at:
+ params.append(f"start_at={start_at}")
+ if end_at:
+ params.append(f"end_at={end_at}")
+ if params:
+ uri += "?" + "&".join(params)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not isinstance(start_at, str): | |
| start_at = timestamp(start_at) | |
| if start_at: | |
| uri += '?start_at={start_at}'.format(start_at=start_at) | |
| if not isinstance(end_at, str): | |
| end_at = timestamp(end_at) | |
| if end_at: | |
| uri += '?end_at={end_at}'.format(end_at=end_at) | |
| # Normalize timestamps only when provided; then build query string once. | |
| if start_at is not None and not isinstance(start_at, str): | |
| start_at = timestamp(start_at) | |
| if end_at is not None and not isinstance(end_at, str): | |
| end_at = timestamp(end_at) | |
| params = [] | |
| if start_at: | |
| params.append(f"start_at={start_at}") | |
| if end_at: | |
| params.append(f"end_at={end_at}") | |
| if params: | |
| uri += "?" + "&".join(params) |
🤖 Prompt for AI Agents
In trakt/sync.py around lines 554 to 563, the code calls
timestamp(start_at)/timestamp(end_at) without guarding against None and appends
query params using '?' twice; change logic to only call timestamp when value is
not None, collect params into a list (e.g. add "start_at=..." and "end_at=..."
entries only when present), then append them to uri once using '?' + '&'. Ensure
you convert non-string datetime-like inputs via timestamp before adding to the
params list so timestamp(None) is never invoked.
| for d in data: | ||
| if 'movie' in d: | ||
| from trakt.movies import Movie | ||
|
|
||
| results.append(Movie(**d.pop('movie'))) | ||
| elif 'show' in d: | ||
| from trakt.tv import TVShow | ||
|
|
||
| results.append(TVShow(**d.pop('show'))) | ||
|
|
||
| yield results |
There was a problem hiding this comment.
Return TVEpisode for episode history entries (currently returns TVShow)
History responses include an episode object with a companion show. Checking show first maps episodes to TVShow incorrectly.
Apply this diff:
results = []
for d in data:
if 'movie' in d:
from trakt.movies import Movie
results.append(Movie(**d.pop('movie')))
- elif 'show' in d:
- from trakt.tv import TVShow
-
- results.append(TVShow(**d.pop('show')))
+ elif 'episode' in d:
+ from trakt.tv import TVEpisode
+ show = d.pop('show', {}) # expected for episodes
+ episode = d.pop('episode')
+ results.append(TVEpisode(
+ show.get('title', None),
+ show_id=(show.get('ids') or {}).get('trakt'),
+ **episode
+ ))
+ elif 'show' in d:
+ from trakt.tv import TVShow
+ results.append(TVShow(**d.pop('show')))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for d in data: | |
| if 'movie' in d: | |
| from trakt.movies import Movie | |
| results.append(Movie(**d.pop('movie'))) | |
| elif 'show' in d: | |
| from trakt.tv import TVShow | |
| results.append(TVShow(**d.pop('show'))) | |
| yield results | |
| results = [] | |
| for d in data: | |
| if 'movie' in d: | |
| from trakt.movies import Movie | |
| results.append(Movie(**d.pop('movie'))) | |
| elif 'episode' in d: | |
| from trakt.tv import TVEpisode | |
| # Extract the parent show info (always present for episodes) | |
| show = d.pop('show', {}) | |
| episode = d.pop('episode') | |
| results.append(TVEpisode( | |
| show.get('title', None), | |
| show_id=(show.get('ids') or {}).get('trakt'), | |
| **episode | |
| )) | |
| elif 'show' in d: | |
| from trakt.tv import TVShow | |
| results.append(TVShow(**d.pop('show'))) | |
| yield results |
🤖 Prompt for AI Agents
In trakt/sync.py around lines 567 to 577, the loop checks for 'show' before
'episode' causing episode history entries to be mapped to TVShow instead of
TVEpisode; update the conditional order to check for 'episode' (and construct a
TVEpisode from d.pop('episode') using from trakt.episodes import TVEpisode)
before checking for 'show', keep the existing handling for 'movie' unchanged,
and ensure results.append uses the appropriate popped object for each branch.
1 participant
FIrst try to add get_history endpoint (https://trakt.docs.apiary.io/#reference/sync/get-history/get-watched-history), relative to #89
I tried to follow the coding style of the project, hope I did good !