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

Issue 36 #215

Closed
wants to merge 2 commits into from
Closed

Issue 36 #215

wants to merge 2 commits into from

Conversation

Naliath
Copy link
Collaborator

@Naliath Naliath commented Mar 25, 2017

Should add feature #36

@LordMike Can you review and up the nuget version, not sure what versioning you want to use.

I also noticed they are starting on a v4 of the api but it's only in the very early stages.

I reran all the tests and 14 out of 220 are failing atm for various reasons. Most of it seems data related, not seeing anything serious atm. Might find some more time this weekend to clean those up.

Naliath added 2 commits March 24, 2017 23:27
… the separate season search object in favor of a generic object that is patched with additional info later on. Added and fixed related tests to deal with the new method and structure.
@LordMike
Copy link
Collaborator

Haven't read the code yet.

Is this grouped thing only for very specific methods, or is it a general thing? Do we know if this changes immensely with v4?

I'm having a hard time wrapping my head around how to do it in a statically typed world, as we pretty much can't handle types that change at runtime... although, we can workaround it.

I've seen this in one other place. In general, you have some thing thing that can exist 0..N times in an object (A season for a TV show). But we never know if we get 0, 1 or N seasons.. So what we could do is hide the list, and expose two methods: GetCount() and GetSeason(INDEX) ... (Or just expose an IEnumerable).

So basically, instead of handline 0, 1 or N seasons, we always handle N and the rest will follow (Count could be 0). For compatibility, we could provide an Extension method that re-adds the property(ies) for "1 Season".

This could extend to other types as well.

@Naliath
Copy link
Collaborator Author

Naliath commented Mar 26, 2017

So basically you can include any call that belongs to the same root and include the result of that call in the response. For example you could make a call with this in the append_to 'season/1,season/1/external_ids' then you will get back
{
"backdrop_path": "/1PktoE5ZhqDscVQwVK35YgOrcAh.jpg",
... more tv show stuff,
"season/1":{season one stuff},
"season/1/external_ids": {season one external id's}
}

FYI this is the same system we use to include the images and what not, it just works on dynamic content as well as the static "/images"

The annoying part of this is that the additional requests go in the tv response object. This makes it difficult since you can not assume that any not parsed property is an appended object to be mapped. You need a way to determine the object type to try to map it to.

Right now for this instance I simply removed the separate tvseason object we where only using here and I'm patching them with the details as required. Similar to what we are doing for the other requests. Since we do not know the number of seasons in advance I am just requesting all I can given the available space in the append param (max 20). The api just ignores the seasons that do not exist.

All in all I think the current solution will cover the majority of the cases. But I agree that a generalized system would be better but more that likely would require more thought to be put in by the consumer of the client than just setting a boolean like he can now.

@LordMike
Copy link
Collaborator

The patching up I've always seen as a giant hack ... :/

Deserializing these objects will be a horrendous effort - especially if support explodes and suddenly works for a number of other things... :/

@Naliath
Copy link
Collaborator Author

Naliath commented Mar 27, 2017

Problem is that there isn't really a way to do it cleanly (that I can think of) that does not involve a shit ton of objects and isn't a pain to consume as a client.

The append to has been there since the start, and is what we use for the level one includes aka the tvshow methods etc. I think we might be able to come up with something to do this in relatively generic way. Just need to play around with the jobject some more.

For me priority one should be that the wrapper interface is easy to use and preferably gives access to all of the api capabilities.

To that end I would like to support stuff like what this PR included. If you want to do really advanced includes that we can not add transparently you are better off just getting our source and extending it for your use case or just roll your own.

Question will be what v4 will bring :). Atm is seems to be more around the other entities, not the core movie and series objects.

@LordMike LordMike closed this Jun 30, 2020
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.

2 participants