Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[python-pytrakt] Refactor: Add IdsMixin to replace extract_ids() utility method #186

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Jan 15, 2022

Extracting the "ids" on each response is tedious, and such duplication results users being confused:

Also, the existing extraction is inconsistent (Not applied for TVShow and Movie in this example):

  • PyTrakt/trakt/sync.py

    Lines 331 to 342 in 8a6d4f1

    for d in data:
    if 'episode' in d:
    from trakt.tv import TVEpisode
    show = d.pop('show')
    extract_ids(d['episode'])
    results.append(TVEpisode(show.get('title', None), **d['episode']))
    elif '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')))

This will make the ids extraction automatic, and prevent updating the attributes directly.

Additionally it came out that properties like tmdb_id, imdb_id, trakt_id are unused. they are initialized as None in the constructor but never any other value. these are commented with @deprecated and are to be removed in 4.x.

  • Person
  • Movie
  • Calendar
  • SearchResult
  • TVShow
  • TVEpisode
  • TVSeason
  • UserList

TODO:

@glensc
Copy link
Contributor Author

glensc commented Jan 15, 2022

It's actually more confusing than that, seems the _id suffixed ones are abandoned:

as the ids property returns without the "_id":

  • PyTrakt/trakt/people.py

    Lines 70 to 76 in 8a6d4f1

    @property
    def ids(self):
    """Accessor to the trakt, imdb, and tmdb ids, as well as the trakt.tv
    slug
    """
    return {'ids': {'trakt': self.trakt, 'slug': self.slug,
    'imdb': self.imdb, 'tmdb': self.tmdb}}

@glensc glensc force-pushed the remove_extract_ids branch from 46db657 to 9a199c6 Compare January 15, 2022 21:36
@glensc
Copy link
Contributor Author

glensc commented Jan 15, 2022

Also, the existing extraction is inconsistent. not applied for TVShow and Movie in this example:

  • PyTrakt/trakt/sync.py

    Lines 331 to 342 in 8a6d4f1

    for d in data:
    if 'episode' in d:
    from trakt.tv import TVEpisode
    show = d.pop('show')
    extract_ids(d['episode'])
    results.append(TVEpisode(show.get('title', None), **d['episode']))
    elif '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')))

@glensc glensc force-pushed the remove_extract_ids branch 5 times, most recently from 2643789 to 2157af6 Compare January 15, 2022 23:25
@glensc
Copy link
Contributor Author

glensc commented Jan 15, 2022

Can't finish UserList as it uses namedtuple:

E   TypeError: Multiple inheritance with NamedTuple is not supported

@glensc
Copy link
Contributor Author

glensc commented Jan 16, 2022

A change is needed to namedtuple definitions (UserList):

  • remove 'trakt', 'slug'
  • add 'ids'

since namedtuple doesn't allow optional values, this would be a breaking change, meaning this needs to land in 4.x

@glensc glensc force-pushed the remove_extract_ids branch 3 times, most recently from 1cbaa2d to 1a5a46b Compare January 16, 2022 01:38
@glensc glensc force-pushed the remove_extract_ids branch from d565a62 to 008c34c Compare January 16, 2022 01:50
@glensc
Copy link
Contributor Author

glensc commented Jan 16, 2022

This is ready now, but not sure if this breakage is big enough that this needs to go to 4.x?

if you're not creating UserList(...dict...) yourself, you're not affected

@glensc
Copy link
Contributor Author

glensc commented Nov 29, 2022

External projects calling extract_ids(data) directly need to omit it after this PR.

@glensc
Copy link
Contributor Author

glensc commented Dec 11, 2022

@glensc glensc changed the title Refactor: Add IdsMixin to replace extract_ids() utility method [python-pytrakt] Refactor: Add IdsMixin to replace extract_ids() utility method Dec 11, 2022
@glensc glensc mentioned this pull request Dec 11, 2022
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.

1 participant