search

jellyfin / TMDbLib

C#.Net library for TheMovieDB
MIT License
344 stars 128 forks source link

Update the AggregateCredits implementation #412

Closed electroflame closed 1 year ago

electroflame commented 1 year ago

This fixes my open issue #410.

Due to the changes in returned data structures, I created new classes (CastAggregate, CrewAggregate, and a CreditsAggregate) that mirror the established Cast, Crew, and Credits classes. Additionally, there are two extra classes (CastRole and CrewJob) that were required to fully map the new roles and jobs arrays returned by TMDB.

I've tested aggregate Cast data pretty extensively, but only done a basic test on aggregate Crew data (basically just making sure the data maps correctly). Everything looks good on my end, but if you see anything or have any suggestions I'm open to any changes. I wasn't entirely sure on your naming conventions, so I tried to play it safe based on what was already there.

LordMike commented 1 year ago

Is there no overlap with the existing Crew / Credit / Cast classes?

Maybe shared properties could be added to new CrewBase/CreditBase/.. classes?

electroflame commented 1 year ago

There's definitely overlap (most of the properties, actually).

Setting up base classes would be my preference as well, actually. I just remembered reading in issue #364 where you mentioned being burned by base classes, so I decided to keep them separate.

In this case, I think base classes are probably justified, since we're talking about sticking with generally like-for-like structures (i.e. a CastBase to a Cast and CastAggregate) whereas in the aforementioned issue it was more of a shared base between different objects (i.e. a shared base for Cast and Crew).

I think I agree with both your previous justification and your current recommendation. Looking at it, I'll definitely set up a CrewBase and CastBase as that makes sense. There's still quite a bit of overlap between Cast and Crew, so you could potentially set up a shared base, but I think that's beyond the scope of this pull request (and your previous justification is still sound, I think).

I'll refactor this real quick, make sure it still works, and push an update.

electroflame commented 1 year ago

Alright, I refactored the Cast and Crew classes to use new CastBase and CrewBase classes.

I didn't refactor Credits or CreditsAggregate, as even though they share Id, their implementation of Cast and Crew differ, due to using different classes. It's possible to do that, but since they return different data it seemed less confusing to keep them separate.

LordMike commented 1 year ago

Looks good :)

electroflame commented 1 year ago

Great, thanks for merging it! I'll close the original issue now, too.