Skip to content

add missing trakt fields from trakt_list#1573

Merged
gazpachoking merged 5 commits intoFlexget:developfrom
Andy2244:trakt_fix
Jan 5, 2017
Merged

add missing trakt fields from trakt_list#1573
gazpachoking merged 5 commits intoFlexget:developfrom
Andy2244:trakt_fix

Conversation

@Andy2244
Copy link
Copy Markdown
Contributor

Motivation for changes:

Some trakt fields where not set for trakt_list imports

Detailed changes:

added: trakt_movie_name, trakt_movie_year, series_year, trakt_series_name, trakt_series_year

I also noticed that there are multiple ways of handling the year and it looks somewhat inconsistent between plugins.
year
series_year
movie_year

Copy link
Copy Markdown
Member

@gazpachoking gazpachoking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the series_ ones, looks good to me.

trakt_id = item['show']['ids']['trakt']
listed_series[trakt_id] = {
'series_name': '%s (%s)' % (item['show']['title'], item['show']['year']),
'series_year': item['show']['year'],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should set this. The series_ fields are meant to come from the series plugin with no lookups required.

Copy link
Copy Markdown
Contributor Author

@Andy2244 Andy2244 Dec 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean movie_year is also handled this way, so why is series_ a exception? Why not fill in all valid information we can get at a particular input stage?
As example i don't use the series plugin and feed the trakt list directly to my search plugin.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we have more of a built in concept of year for the movie stuff, and the offline parser fills it. With series, due to historical reasons, we have not had an explicit year involved, it's always just been lumped in in the series title.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My argument would be that it already fills in the series_name using the year from trakt. so why should it not also fill in the series_year, so those two fields are consistent.

I can remove the fill in, just seems strange to me that it uses the year in the name and trakt_year fields, yet omits it for the general field?

Copy link
Copy Markdown
Member

@gazpachoking gazpachoking Dec 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why should it not also fill in the series_year, so those two fields are consistent.

Because it seems to imply that the series plugin has some sort of support for series_year, when it doesn't. I think it would be confusing to see that field populated for your series when you enable the trakt_lookup plugin, but not have it be there otherwise. When you enable the trakt_lookup plugin, you can explicitly access the year via trakt_series_year.

EDIT: The movie_ namespace is a bit different, because there isn't a movie plugin, those fields are just a gentleman's agreement that all the plugins dealing with movies look at. And they all know about the movie_year. No plugins dealing with series know about series_year, so I don't think we should add it anywhere unless we intend to make it consistent across every series plugin, including the actual series plugin.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are the expert and i don't really care, since i don't use the field either.
However here is something i find strange. Why is filling a data field dependent on use knowledge and not available information and trusted source? I mean i trust the trakt year, so copying it to the generic field seems valid to me. In comparison i don't trust the newznab tagged "tvairdate", so i did not copy this date to the generic field and left it in the newznab namespace. Yet i trust the "size" and copy this to 'content_size'. In the same sense we trust the "tvdb_id" comming from trakt, so we copy it, instead of leaving it in the trakt namespace.

So to me if i trust the source, i have no problem populating the generic fields, if the information is available. Just my personal thoughts, so just tell me if i should remove it or not so we can get this merged.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's got nothing to do with 'trusted source'. The field series_year is not used anywhere, so there's no point starting now. series plugin needs a bigger overhaul and that is beyond the scope of this PR.

I can see that it is set here

entry['series_year'] = year
, but it shouldn't be.

Copy link
Copy Markdown
Contributor Author

@Andy2244 Andy2244 Dec 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user could use this field via set/if/sort plugins, custom.render(). Its just convenient to have general series_year field, especially since omitting or not omiting the year from title depends on the task flow.

So just to give a example, you want to set the title for a task to 'series_name' + year, assuming you normally omit this from you inputs. File rename task or custom search tasks come to mind. Now instead of using 'series_year' the same way you could use 'movie_year', you need specific information what metadata_lookup was used and successfully did the lookup and than separately check the namespace fields like imdb_year, trakt_series_year, tvmaze_series_year. My argument is why do i care which meta lookup populated this field as long as its valid. If the user has multiple meta lookup defined, why should he care which of those had a successful lookup and populated the year field, so it can be used?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get your intentions, but as we've said, it's for historical reasons. Doesn't mean you don't have a valid point, it's just not something that belongs in this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oki changed

@gazpachoking
Copy link
Copy Markdown
Member

Looks like tests need a bit of updating as well.

@Andy2244
Copy link
Copy Markdown
Contributor Author

Andy2244 commented Jan 4, 2017

Are there still problems with this PR?

@cvium
Copy link
Copy Markdown
Contributor

cvium commented Jan 4, 2017

Well, you haven't fixed the tests https://travis-ci.org/Flexget/Flexget/jobs/185735702

@gazpachoking gazpachoking merged commit 1ffb140 into Flexget:develop Jan 5, 2017
@gazpachoking
Copy link
Copy Markdown
Member

Thanks!

@Andy2244 Andy2244 deleted the trakt_fix branch January 6, 2017 12:59
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.

3 participants