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

Improve multi-episode and season snatches. Fixes #229. Fixes #4750 #4675

Merged
merged 17 commits into from
Aug 1, 2018

Conversation

medariox
Copy link
Contributor

@medariox medariox commented Jul 15, 2018

  • PR is based on the DEVELOP branch
  • Don't send big changes all at once. Split up big PRs into multiple smaller PRs that are easier to manage and review
  • Read the contribution guide

TODO:
- [ ] Add support for cached season results (for another PR)

The old code was extremely error-prone, tried to do too much and was causing unexpected behaviors. The poor implementation was probably caused by the fact that season results didn't have any episodes unlike single and multi-episodes. As a bonus, it fixes the oldest bug #229 🎉

@medariox medariox added this to the 0.2.7 milestone Jul 15, 2018
@medariox medariox added Concluded Needs testing Requires testing to make sure it's working as intended and removed In progress labels Jul 17, 2018
@medariox medariox modified the milestones: 0.2.7, 0.2.8 Jul 19, 2018
@medariox medariox changed the title Improve multi-episode and season snatches. Fixes #229 Improve multi-episode and season snatches. Fixes #229 #4750 Jul 25, 2018
all_wanted = False
else:
any_wanted = True
for ep_obj in candidate.episodes:
Copy link
Contributor

@sharkykh sharkykh Jul 27, 2018

Choose a reason for hiding this comment

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

How do you feel about this?

wanted_check = (
    series_obj.want_episode(ep_obj.season, ep_obj.episode, candidate.quality, down_cur_quality)
    for ep_obj in candidate.episodes
)
all_wanted = all(wanted_check)
any_wanted = any(wanted_check)

And since all_wanted/any_wanted are only used once,
lose the variables and just if all(wanted_check): ... elif not any(wanted_check):
It would also make the checks lazy.

I didn't test my code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's indeed better than the legacy code.

@@ -247,21 +247,24 @@ def find_search_results(self, series, episodes, search_mode, forced_search=False

results = {}
items_list = []
season_search = (
len(episodes) > 1 or manual_search_type == 'season'
) and search_mode == 'sponly'
Copy link
Contributor

Choose a reason for hiding this comment

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

The line breaks don't help readability in this case, IMO.

@@ -861,15 +861,6 @@ def collect_multi_candidates(candidates, series_obj, episodes, down_cur_quality)
elif len(cur_result.episodes) > 1:
multi_candidates.append(cur_result)

# If this is a torrent all we can do is get the entire torrent,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should highlight this change in the changes & news.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to the changelog.

"""
found_results = {}

"""Check providers for details on wanted episodes."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you complete this docstring please?

"""
Check providers for details on wanted episodes.

:param force: ???
:return: a list of ???
"""

if not Quality.is_higher_quality(single_results[episode_no].quality,
best_result.quality, allowed_qualities,
preferred_qualities):
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

Log a debug message when a result is skipped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't add this. It's going to be very spammy.

@sharkykh sharkykh changed the title Improve multi-episode and season snatches. Fixes #229 #4750 Improve multi-episode and season snatches. Fixes #229. Fixes #4750 Jul 27, 2018
p0psicles
p0psicles previously approved these changes Aug 1, 2018
@p0psicles
Copy link
Contributor

@sharkykh would you mind approving? I think its good

CHANGELOG.md Outdated
#### Fixes
- Fixed error when changing episode status from episode status management ([#4783](https://github.com/pymedusa/Medusa/pull/4783))
- Fixed multi-episode snatches not being marked as snatched in history ([#229](https://github.com/pymedusa/Medusa/issues/229))
- Fixed whole seasons being downloaded as multi-episode replacement ([#4750](https://github.com/pymedusa/Medusa/issues/4750))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please merge develop and put these in the "Unreleased" section.
If we start setting version numbers and dates too soon we'll have to keep updating them until we actually release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, thank you for including #4783 which I forgot to add!

@p0psicles p0psicles merged commit 55bfdaf into develop Aug 1, 2018
@p0psicles p0psicles deleted the feature/imp-multi-snatches branch August 1, 2018 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Concluded Enhancement Needs review Needs testing Requires testing to make sure it's working as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants