Skip to content

[change] next_series_* - Emit following season if latest download is a season pack#1794

Closed
tubedogg wants to merge 6 commits intoFlexget:developfrom
tubedogg:ns-season-packs
Closed

[change] next_series_* - Emit following season if latest download is a season pack#1794
tubedogg wants to merge 6 commits intoFlexget:developfrom
tubedogg:ns-season-packs

Conversation

@tubedogg
Copy link
Copy Markdown
Contributor

@tubedogg tubedogg commented Apr 17, 2017

Motivation for changes:

Align behavior when the last release in series history is a season to what has always occurred when the last release in series history is an episode.

Detailed changes:

  • Given a last release in series history of a season pack S02, now emits S03 for next_series_seasons or S03E01 for next_series_episodes, as opposed to not emitting anything.
  • If the latest known season is completed in the database, next_series_* tries to review the following season for emit. Currently in this scenario it will emit nothing if from_start or backfill is not set. This change causes it to emit the following season.
  • Also fixed a tiny bug in next_series_seasons on line 129 (search_entry does not take an episode number in next_series_seasons).

Addressed issues:

Resolves #1782.

Log and/or tests output (preferably both):

Given a series download history of S03 and S06 for My Show...
next_series_seasons:

2017-04-17 18:19 DEBUG    series        catchup_btn_search_seasons connecting series My Show to task catchup_btn_search_seasons
2017-04-17 18:19 DEBUG    series        catchup_btn_search_seasons no downloaded episodes found for series 'My Show', season: None, downloaded: True
2017-04-17 18:19 DEBUG    series        catchup_btn_search_seasons no downloaded episodes found for series 'My Show', season: 7, downloaded: True
2017-04-17 18:19 DEBUG    series        catchup_btn_search_seasons no downloaded season packs found for series 'My Show', season: 7, downloaded: True
2017-04-17 18:19 VERBOSE  discover      catchup_btn_search_seasons Discovering 1 titles ...
2017-04-17 18:19 DEBUG    discover      catchup_btn_search_seasons My Show S07 -> No previous run recorded

next_series_episodes:

2017-04-17 18:19 DEBUG    series        catchup_btn_search_eps connecting series My Show to task catchup_btn_search_eps
2017-04-17 18:19 DEBUG    series        catchup_btn_search_eps no downloaded episodes found for series 'My Show', season: None, downloaded: True
2017-04-17 18:19 DEBUG    series        catchup_btn_search_eps no downloaded episodes found for series 'My Show', season: 7, downloaded: True
2017-04-17 18:19 DEBUG    series        catchup_btn_search_eps no downloaded season packs found for series 'My Show', season: 7, downloaded: True
2017-04-17 18:19 VERBOSE  discover      catchup_btn_search_eps Discovering 1 titles ...
2017-04-17 18:19 DEBUG    discover      catchup_btn_search_eps My Show S07E01 -> No previous run recorded

@tubedogg
change next_series_* - Emit following season if latest download is a …
8edb958 
…season pack

Given a last release in series history of a season pack, now emits S04
for next_series_seasons or S04E01 for next_series_episodes as opposed
to not emitting anything.
@jawilson jawilson requested review from jawilson and liiight April 18, 2017 01:06
jawilson
jawilson requested changes Apr 18, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@jawilson jawilson left a comment

Choose a reason for hiding this comment

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

Please add some unit tests for this behavior.

@tubedogg
Unit tests
0e6cb0b 
Test next_series_episodes/next_series_seasons without begin to ensure
S03/S03E01 is emitted given history of S01 and S02.
@tubedogg
Copy link
Copy Markdown
Contributor Author

tubedogg commented Apr 18, 2017 •
edited
Loading

Done. I think I did it right.

Edit: I don't know how to run the tests, though.

tubedogg and others added 3 commits April 17, 2017 21:30
@tubedogg
Fix test
8cba582 
Add config for next_series_episodes
@jawilson
Revert unit tests
6db8e33 
This reverts commit 8cba582.
This reverts commit 0e6cb0b.
@jawilson
Add next_series_* unit tests for season packs
8e74ad0
@jawilson jawilson mentioned this pull request Apr 18, 2017
Merged
@jawilson
Copy link
Copy Markdown
Member

jawilson commented Apr 18, 2017

@tubedogg, I added some more clear tests in tubedogg#1. If you merge that PR it will be included here.

@liiight
Copy link
Copy Markdown
Member

liiight commented Apr 18, 2017

i'm not sure about this.
The scenario we want to handle is series that has downloads, but doesn't have a begin episode set right? So what i thought about doing is just add a property to series that return True in case the series has any download, and the just add that condition to the backfill check line.
This way is kinda hacking the loop, which is weird IMO.

Also, next_series_X already have existing tests under test_discovery, which i agree is not the ideal place, but they already exist.

@tubedogg
Copy link
Copy Markdown
Contributor Author

tubedogg commented Apr 18, 2017

I guess I don't see the difference (though I'm also not entirely sure why the from_start/backfill line 170 isn't an elif instead of being enclosed in the else block) but I don't really have a strong opinion one way or the other.

I do see one small problem with what I did in that latest_season_inc will always be True if there were ever any downloads, as opposed to only if the season was incremented beyond existing downloads, but functionally it doesn't appear that it would actually matter since if if the season wasn't incremented, latest would have a result and it would never get to my conditional anyway.

@liiight
Copy link
Copy Markdown
Member

liiight commented Apr 18, 2017

I may very well be that the end result is the same, but when dealing with something as already as complex as this, I rather add as little code as possible, and make it as obvious as possible.

@jawilson
Copy link
Copy Markdown
Member

jawilson commented Apr 18, 2017

Testing via discover is indirectly testing and definitely not good, we should remove the non-discover specific tests from there and move them to the proper direct tests.

There are a few cases that need to be handled:

  • If a new future season pack (S02) is downloaded (even if it is the first item to be downloaded) then next_series_episodes should emit the first episode of the next season (S03E01) and series_next_seasons should emit the next season (S03).
  • If a new future season pack (S02) is downloaded (even if it is the first item to be downloaded) and backfill enabled all previous season (premieres) should be emitted by the respective plugin
  • In the previous scenerio if series_begin is set, then all previous season (premieres) back to the series_begin should be emitted.

@tubedogg tubedogg changed the title change next_series_* - Emit following season if latest download is a season pack [change] next_series_* - Emit following season if latest download is a season pack Apr 20, 2017
@liiight
Copy link
Copy Markdown
Member

liiight commented Apr 30, 2017

Thanks for this @tubedogg , but i think ill go ahead with my idea on this one.
If you or @jawilson wants to sort out the tests after I merge #1797 that would be highly appreciated.

@liiight liiight closed this Apr 30, 2017
tubedogg added a commit that referenced this pull request May 5, 2017
@tubedogg
Unit tests
8779377 
Moving tests that @jawilson provided in #1794, plus additional.
liiight added a commit that referenced this pull request May 6, 2017
@liiight
[add] Season packs phase 2 (#1797)
5d3fd5a 
* Changed series forget to support seasons as well

* Correctly sort show entities

* Fixed series forget cli

* Added log message when looking for season pack using tvdb

* Removed the pointless is_season_pack property

* Added ability to send multiple entities for delete/forget

* Catch LookupError as well

* Forget downloaded season packs

* Fixed series show sort

* Added logic to emit next episode without series begin

* Added logic to next_series_seasons

* Fixed next_series_logic

* Fixed crash with series begin and series show

* Fixed wrong assignment

* Removed redundant has_history

* Unit tests

Moving tests that @jawilson provided in #1794, plus additional.

* Fix tests

* Really fix tests

The tests involving series.begin will still fail until that issue is
resolved in the actual plugins in a separate commit.

* Check series.begin earlier

Resolves an issue where a season prior to series.begin could be
processed before the break check is reached

* Set low_season to series.begin

* Final location of series.begin check

Probably

* latest_season cannot be lower than series.begin

* Only change low_season if series.begin.season > 1

* Finally fix tests; parametrize new tests

* Resolve issues with series.begin

- low_season has to be 1 lower than the actual last season to test for
range() to work correctly
- Only try to check if begin season is completed while evaluating the
begin season (otherwise, with gaps in history, it will only ever emit
the begin episode)

* Additional tests

- Gaps between season packs
- from_start enabled

* Resolve issues with series.begin (ns_seasons)

- low_season has to be 1 lower than the actual last season to test for
range() to work correctly
- Only try to check if begin season is completed while evaluating the
begin season (otherwise, with gaps in history, it will only ever emit
the begin episode)

* Additional tests (seasons)

- Gaps between season packs
- from_start enabled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jawilson jawilson jawilson approved these changes

@liiight liiight Awaiting requested review from liiight

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Behavior of next_series_episodes with release history of season packs vs episodes

3 participants

@tubedogg @jawilson @liiight