[Feature] added get_history function #2 #92
Conversation
added get_history function
Considering coderabbit return on my first PR, I changed TVShow to TVEpisode as get_history returns episodes, not shows. I edited it instead of approving coderabbit proposition beacause it added unecessary (IMO) stuff.
WalkthroughAdded a new function get_history in trakt/sync.py (decorated with @get) to retrieve sync/history with optional type, id, and time-range filters. It constructs the URI with path segments and query parameters, invokes the API, and maps results to Movie or TVEpisode objects. Query param assembly uses separate ?start_at and ?end_at fragments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant S as Sync.get_history()
participant A as Trakt API (sync/history)
participant M as Mapper (Movie/TVEpisode)
C->>S: get_history(list_type?, id?, start_at?, end_at?)
Note right of S: Build URI: /sync/history[/type[/id]]<br/>Append start_at/end_at as query params
S->>A: GET /sync/history...
A-->>S: JSON history entries
S->>M: Map each entry by type
M-->>S: [Movie | TVEpisode, ...]
S-->>C: Return list
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 (2)
trakt/sync.py (2)
533-533: Prefer not shadowing built-in idRename the parameter and its usages for clarity.
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
533-541: Optional: enforce timezone-aware datetimesIf callers pass naive datetimes, timestamp() will serialize them as-is. Consider normalizing to UTC or documenting the expectation.
📜 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)
TVEpisode(723-970)
🔇 Additional comments (1)
trakt/sync.py (1)
547-553: Confirm: is item id valid without specifying type?Trakt’s docs typically show the form /sync/history/{type}/{id}. If type is required when passing an id, we should guard against item_id without list_type.
Apply this (if confirmed):
uri = 'sync/history' if list_type: uri += '/{}'.format(list_type) - if item_id: + if item_id: + if not list_type: + raise ValueError("When specifying an id, list_type must be 'movies' or 'episodes'.") uri += '/{}'.format(item_id)Would you like me to check the latest Trakt spec and adjust accordingly?
| 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.
Bug: None handling and malformed query string for start_at/end_at
- start_at/end_at are timestamped even when None -> crashes.
- Second query param incorrectly uses ? instead of &.
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)
+ params = []
+ if start_at:
+ if not isinstance(start_at, str):
+ start_at = timestamp(start_at)
+ params.append(f"start_at={start_at}")
+ if end_at:
+ if not isinstance(end_at, str):
+ end_at = timestamp(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) | |
| params = [] | |
| if start_at: | |
| if not isinstance(start_at, str): | |
| start_at = timestamp(start_at) | |
| params.append(f"start_at={start_at}") | |
| if end_at: | |
| if not isinstance(end_at, str): | |
| end_at = timestamp(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-563, the code timestamps start_at/end_at even
when they are None (causing crashes) and appends the second query param with '?'
instead of '&', producing a malformed query string; fix by only calling
timestamp(...) if the value is not None, build the query string safely by
collecting params into a list (e.g., params = [] then if start_at is not None:
params.append(f"start_at={timestamp(start_at) if not isinstance(start_at, str)
else start_at}") and similarly for end_at) and then append them to uri as '?' +
'&'.join(params) only if params is non-empty (avoiding multiple '?' and skipping
None values).
| elif 'episode' in d: | ||
| from trakt.tv import TVEpisode | ||
|
|
||
| results.append(TVEpisode(**d.pop('episode'))) | ||
|
|
There was a problem hiding this comment.
Bug: TVEpisode constructed without required show context
TVEpisode requires show context; constructing with only episode dict will raise or produce an invalid object. Follow the existing pattern used elsewhere in this module.
Apply this diff:
- elif 'episode' in d:
- from trakt.tv import TVEpisode
-
- results.append(TVEpisode(**d.pop('episode')))
+ elif 'episode' in d:
+ from trakt.tv import TVEpisode
+ show = d.pop('show')
+ ep = d.pop('episode')
+ results.append(
+ TVEpisode(
+ show.get('title', None),
+ show_id=show['ids'].get('trakt'),
+ **ep
+ )
+ )📝 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.
| elif 'episode' in d: | |
| from trakt.tv import TVEpisode | |
| results.append(TVEpisode(**d.pop('episode'))) | |
| elif 'episode' in d: | |
| from trakt.tv import TVEpisode | |
| show = d.pop('show') | |
| ep = d.pop('episode') | |
| results.append( | |
| TVEpisode( | |
| show.get('title', None), | |
| show_id=show['ids'].get('trakt'), | |
| **ep | |
| ) | |
| ) |
🤖 Prompt for AI Agents
In trakt/sync.py around lines 572-576, the code constructs TVEpisode with only
the episode dict which lacks required show context; instead, pop the episode
dict and the associated show dict (as done elsewhere), construct a TVShow object
from the show dict, then construct the TVEpisode passing the episode fields plus
show=that TVShow instance; also add/import TVShow at the top if not already
imported so the episode has proper show context.
1 participant
Considering coderabbit return on previous commit, I changed TVShow to TVEpisode as get_history returns episodes, not shows. I edited it instead of approving coderabbit proposition beacause it added unecessary (IMO) stuff.
It resulted in 2 differents PR, I don't know if I could have done it differently I'm no github pro !