Generic upgrade and timeframe plugins by stevezau · Pull Request #2036 · Flexget/Flexget

stevezau · 2017-12-09T05:45:11Z

Motivation for changes:

Currently, flexget does not support the concept of movie upgrade/timeframes. Series upgrades/timeframe are possible but the code is messy. There is no easy/generic way to upgrade entry qualities or wait for a timeframe before accepting the best quality.

Detailed changes:

The following plugins are meant to support generic timeframes/upgrades by tracking new field id. This field is set by metainfo plugins.

upgrade plugin https://flexget.com/Plugins/upgrade
timeframe plugin https://flexget.com/Plugins/timeframe
best_quality plugin https://flexget.com/Plugins/best_quality

cvium · 2017-12-09T07:24:07Z

flexget/plugins/filter/upgrade.py

+
+    def prepare_config(self, config):
+        if not config or isinstance(config, bool):
+            return {'identified_by': 'auto', 'on_lower': 'skip', 'tracking': True}


I'd rather the defaults were set only in prepare_config with dict.setdefault(). We don't really want to maintain the default values in two places

I wish there was a better way.. I wish we didn't near prepare_config at all. But we need it because it's oneOf

I don't mind prepare_config.

cvium · 2017-12-09T07:33:42Z

flexget/plugins/filter/upgrade.py

+
+                # Action lower qualities
+                if config['on_lower'] != 'skip':
+                    entry_actions = {


Minor thing: You're recreating this dict for every identifier.

moved to module level

liiight · 2017-12-11T09:54:50Z

flexget/plugins/filter/upgrade.py

+    # Group by Identifier
+    for entry in entries:
+        identifier = entry.get('id') if identified_by == 'auto' else entry.render(identified_by)
+        if identifier:


Just my personal preference, I prefer to keep the flow control leaner:

if not identifier: log.debug('No identifier found for', entry['title']) continue grouped_entries[identifier.lower()].append(entry)

just a suggestion obviously

liiight · 2017-12-28T10:50:47Z

flexget/plugins/filter/upgrade.py

+                target_downloaded = False
+
+                if config['target']:
+                    target_quality = qualities.Quality(config['target'])


what is returned if the user inserted a quality requirement and not a quality?

based on my tests it returns the highest quality

liiight · 2017-12-28T10:54:03Z

flexget/plugins/filter/upgrade.py

+                    target_quality = qualities.Quality(config['target'])
+                    target_requirement = qualities.Requirements(config['target'])
+                    # Filter out entries within target
+                    entries = list(filter(lambda e: target_requirement.allows(e['quality']), entries))


i like list comprehension here better too...

entries = [e if target_requirement.allows(e['quality']) for e in entries]

but it should filter out ones that don't match? that will just include them all again.. you are missing the else part..

since it runs on the same variable it'll just recreate it with those matching the statement. no need for else in list comprehension

entries = [e if target_requirement.allows(e['quality']) for e in entries] is not valid tho? You need an else after the if?

oops sorry:
entries = [e for e in entries if target_requirement.allows(e['quality'])]

liiight · 2017-12-28T10:55:09Z

flexget/plugins/filter/upgrade.py

+                        continue
+
+                # We may have no entries within target
+                if len(entries) == 0:


more pythonic to write if not entries

already fixed this, will push later

liiight · 2017-12-28T11:00:20Z

flexget/plugins/filter/upgrade.py

+
+                # Action lower qualities
+                if config['on_lower'] != 'skip':
+                    for entry in entries:


shouldnt you pop the first entry to not iterate over it twice?

not sure it really matters?

well i think itll try to accept it twice, not sure if that's an issue or not

liiight · 2017-12-28T11:05:44Z

flexget/plugins/filter/upgrade.py

+    _quality = Column('quality', String)
+    quality = quality_property('_quality')
+    proper = Column(Boolean)
+    added = Column(DateTime, index=True)


you can set default=datetime.now() here and not handle in via the plugin

ah missed that

liiight · 2017-12-28T11:06:16Z

flexget/plugins/filter/upgrade.py

+                    existing = EntryUpgrade()
+                    existing.id = identifier
+                    session.add(existing)
+                else:


maybe change this to elif

cvium · 2017-12-28T11:29:57Z

flexget/plugins/filter/upgrade.py

+                    for entry in entries:
+                        action = entry_actions[config['on_lower']]
+                        if entry['quality'] > existing.quality and target_downloaded:
+                            msg = 'on_lower %s because already at target quality' % config['on_lower']


These messages sound weird. Maybe create a mapping between the config values and the proper tense. And remove the on_lower portion.

Secondly, I think it would be better to change the if-blocks to

if something: do stuff elif other something: do other stuff elif third something: do third stuff else: continue action(entry, msg)

cvium · 2017-12-28T11:34:38Z

flexget/plugins/filter/upgrade.py

+                    # No existing, skip
+                    continue
+
+                target_downloaded = False


I'm assuming this will be used somehow later, so this is just a reminder that this variable is currently never True.

yeah for later use, still wip

liiight

Maybe some more logging?

liiight · 2017-12-28T14:25:56Z

flexget/plugins/filter/upgrade.py

+                # Action lower qualities
+                if config['on_lower'] != 'skip':
+                    for entry in entries:
+                        if entry == best:


No need for this since you used pop

…oosely (ignore fields that are set by other plugins such as movie_name from imdb_lookup)

liiight · 2018-01-08T10:04:19Z

flexget/plugins/filter/timeframe.py

+    proper_count = Column(Integer, default=0)
+
+    def __init__(self):
+        self.first_seen = datetime.now()


no need for this, you init it in the column declaration

liiight · 2018-01-08T10:04:39Z

flexget/plugins/filter/timeframe.py

+
+    def __init__(self):
+        self.first_seen = datetime.now()
+        self.updated = datetime.now()


you didn't create a corresponding column for updated

liiight · 2018-01-08T10:05:40Z

flexget/plugins/filter/timeframe.py

+        try:
+            self.backlog = plugin.get_plugin_by_name('backlog')
+        except plugin.DependencyError:
+            log.warning('Unable to utilize backlog plugin, so episodes may slip through timeframe.')


add self.backlog = None or it will crash later on

liiight · 2018-01-08T10:06:22Z

flexget/plugins/filter/timeframe.py

+        identified_by = '{{ id }}' if config['identified_by'] == 'auto' else config['identified_by']
+
+        grouped_entries = group_entries(task.accepted + task.undecided, identified_by)
+        if len(grouped_entries) == 0:


if not grouped_entries: is more pythonic

liiight · 2018-01-08T10:07:48Z

flexget/plugins/filter/timeframe.py

+
+                id_timeframe.title = best_entry['title']
+                id_timeframe.quality = best_entry['quality']
+                id_timeframe.title = best_entry.get('proper_count', 0)


you probably meant id_timeframe.proper_count here

liiight · 2018-01-08T10:09:43Z

flexget/plugins/filter/timeframe.py

+        identified_by = '{{ id }}' if config['identified_by'] == 'auto' else config['identified_by']
+
+        grouped_entries = group_entries(task.accepted, identified_by)
+        if len(grouped_entries) == 0:


not grouped_entries

liiight · 2018-01-08T10:10:22Z

flexget/plugins/filter/upgrade.py

+    first_seen = Column(DateTime, default=datetime.now)
+    updated = Column(DateTime, index=True, default=datetime.now)
+
+    def __init__(self):


no need to init, you declared the defaults already

liiight · 2018-01-08T10:10:52Z

flexget/plugins/filter/upgrade.py

+    }
+
+    def prepare_config(self, config):
+        if config is None or config is False:


if not config

liiight · 2018-01-08T10:12:01Z

flexget/plugins/filter/upgrade.py

+        identified_by = '{{ id }}' if config['identified_by'] == 'auto' else config['identified_by']
+
+        grouped_entries = group_entries(task.accepted + task.undecided, identified_by)
+        if len(grouped_entries) == 0:


if not grouped_entries

liiight · 2018-01-08T10:12:31Z

flexget/plugins/filter/upgrade.py

+        identified_by = '{{ id }}' if config['identified_by'] == 'auto' else config['identified_by']
+
+        grouped_entries = group_entries(task.accepted, identified_by)
+        if len(grouped_entries) == 0:


if not grouped_entries

stevezau · 2018-01-08T11:25:35Z

Reviewed by @liiight

stevezau added 3 commits December 9, 2017 16:38

set identifer for entries

4df7db8

initial upgrade plugin

565e5f6

fix tests

17e9dcf

stevezau self-assigned this Dec 9, 2017

stevezau requested review from gazpachoking and liiight December 9, 2017 05:51

cvium reviewed Dec 9, 2017

View reviewed changes

cleanup prepare config

2e91dbf

cvium reviewed Dec 9, 2017

View reviewed changes

minor cleanup on entry_actions

d2d2478

liiight reviewed Dec 11, 2017

View reviewed changes

stevezau added 2 commits December 26, 2017 10:28

cleaner grouping logic

990f4f7

add target support

d0d226f

stevezau changed the title ~~[WIP] Generic upgrade plugin~~ [WIP] Generic upgrade and timeframe plugins Dec 28, 2017

stevezau added 2 commits December 28, 2017 20:41

add upgrade timeframe tests

b00c860

add timeframe support

93ea8fc

liiight reviewed Dec 28, 2017

View reviewed changes

cleanups

5960f6a

cvium reviewed Dec 28, 2017

View reviewed changes

stevezau added 2 commits December 28, 2017 23:23

refactor

08c5bf0

add proper support

e84618f

stevezau changed the title ~~[WIP] Generic upgrade and timeframe plugins~~ Generic upgrade and timeframe plugins Dec 28, 2017

liiight reviewed Dec 28, 2017

View reviewed changes

stevezau added 6 commits December 29, 2017 09:58

add log messages and better on_lower messages

2257a90

fix reject, debug messages and tests

a369917

fix lint error

9f370b1

remove builtin, allow for tracking only

b05262c

Add identifers to plugin parsers/lookups

393f51b

revert identifers for lookup plugins

c7cdc19

stevezau added 22 commits January 6, 2018 14:59

rename class

af31f2e

Fixes for timeframe, tracking and target

0cb0a56

don't query if no ids are found

831a954

remove unused import

3f26ab3

filter out rejected

a727154

timeframe plugin

2fa1172

add on_wait and on_reached actions

dcf7b52

imdb_lookup set id field

3a77096

fix tests

6841550

move group entry to common tool

b468a10

add best_quality filter plugin

c024a17

movie id set only by metainfo_movie. metainfo_movie will set fields l…

e3169eb

…oosely (ignore fields that are set by other plugins such as movie_name from imdb_lookup)

metainfo_series runs if id not set

1ffd8c4

use is_lazy instead of directly accessing store.

69ffcbd

fix tests

b08d7e3

fix default value

4240aaf

better checking if fields are set

b3833ec

fix grouping

c6104cd

change skip to allow, seems better naming

77e5a4a

ignore render errors on group entries

812f80f

expect RenderError instead

49e4df5

fix circular import

cc86d23

stevezau added the Enhancement label Jan 8, 2018

change allow to do_nothing

32888a7

liiight reviewed Jan 8, 2018

View reviewed changes

stevezau added 2 commits January 8, 2018 21:24

cleanups

f579433

fix bug

b03465a

stevezau merged commit 9544f56 into develop Jan 8, 2018

stevezau deleted the generic_upgrade_plugin branch January 8, 2018 11:25

Conversation

stevezau commented Dec 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for changes:

Detailed changes:

Uh oh!

cvium Dec 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liiight left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

stevezau commented Dec 9, 2017 •

edited

Loading

cvium Dec 9, 2017 •

edited

Loading