-
Notifications
You must be signed in to change notification settings - Fork 4
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
platform-slug for get_item_of_type potentially breaking change #10
Comments
I would vote for keeping backwards compatibility. Calling For the options I think that if they are translated directly to query args then the function param should be specific to query parameters or something like |
I am on board with backwards compatibility as well. Let's make queryargs a fourth argument of get_item_of_type. However, we may want to re-address the 'private' issue. It seems that the only item that is ever 'private' is 'assets'. As for the queryargs, I think the [options] as we've done it so far in cases that use get_list_data() is an unnamed array. I also note that the shorthand functions for assets -- get_franchise_assets etc -- have args for 'type' and 'window' that are now doable in the API as query filters. I am not thrilled to do this, but maybe we should put together some new functions that better match the API, keeping the old functions around as deprecated backwards compatibile things. |
PS, I'm not proposing to let the old functions just break. I will make an optional fourth arg for get_item_of_type, an unnamed array of queryargs. I'll add that optional fourth arg to get_episode_assets, get_special_assets, etc. also. |
@acrosman @augustuswm are either of you using the 'private' arg anywhere for 'get_episode' or 'get_special' ? It doesn't seem to do anything -- 'private' only actually applies to assets. But if you're not using it, I'll remove the option, make a special case for assets, and overall simplify our syntax choices. |
I know we do not. We only use it for assets during ingest. |
… private option for get_asset be optional and replaceable with an array
Reread my previous comment about query args. That was mostly about not adding special keys like private to the query args array in fear of collisions. I agree with updating existing functions first. I should be able to find some time to help with this. It might be interesting to look at a simple builder pattern for querys as a way to sidestep trying to match the API endpoint structure. |
Sorry for the delay in responding here. We do not use the private flag either, and I kinda like not having parameters that are more or less magically determining behavior based on type. Writing clear docs that explain it can be hard. |
@augustuswm @acrosman There's an announcement from Jenny Markley
http://digitalsupport.pbs.org/support/discussions/topics/12000014567
that "Franchises, shows, and asset objects that are not available on “All Platforms” will not be returned, unless a platform filter is applied (example /shows/<slug|id>/?platform-slug=partnerplayer)."
All of the 'get_show' etc calls are masks for get_item_of_type, which isn't setup to take extra params beyond 'private=true' to tell it to check against the /edit/ endpoint for unpublished assets.
We can either do a possibly breaking change, where we change the 3rd (optional) argument from 'true'/'false' that gets us the 'private' option to an array, where 'private' is one of the array members, or we make that options array a 4th argument and leave 'private' as the 3rd arg. Whichever choice we make would also carry to 'get_show', 'get_asset', etc.
I propose the latter choice (adding a 4th argument that is an options array) as being the most backwards compatible. It seems less elegant on the face of it, but the 'private=true' thing is changing the endpoint URL, while the options array would be changing the params passed.
The 4th argument would be optional of course.
The text was updated successfully, but these errors were encountered: