Created unique plugin by BackSlasher · Pull Request #1678 · Flexget/Flexget

BackSlasher · 2017-02-09T18:29:17Z

Takes action on all duplicates, except for the first

Motivation for changes:

Redoing #1231 in a separate plugin
Allowing to create unique lists by rejecting second+ items based on a list of fields.
Also created tests

Detailed changes:

New plugin "unique":

unique:
  field: imdb_id

unique:
  field: 
  - series_episode
  - series_name

Addressed issues:

(no tickets)
Personal usecase - select only one entry of each kind (e.g. only one torrent for a movie)

Config usage if relevant (new plugin or updated schema):

New plugin as described above.
Nothing else

Log and/or tests output (preferably both):

nitz@mars:~/projects/flexget/virtualenv (master *+%)$ bin/py.test ~/projects/flexget/flexget/flexget/tests/test_unique.py 
=========================================================================================== test session starts ============================================================================================
platform linux2 -- Python 2.7.11+, pytest-2.9.2, py-1.4.31, pluggy-0.3.1
rootdir: /home/nitz/projects/flexget/flexget, inifile: setup.cfg
plugins: xdist-1.14, cov-2.2.1, capturelog-0.7
collected 6 items 

../flexget/flexget/tests/test_unique.py ......

cvium · 2017-02-09T18:31:19Z

flexget/plugins/filter/unique.py

You should use setdefault() instead

cvium · 2017-02-09T18:31:28Z

flexget/plugins/filter/unique.py

Why the linebreak?

Line seemed too long, wanted to simplify reading it. Will move to one line

Isn't this function only used in one place? Might make more sense to scrap the function then... It only makes it harder to read.

cvium · 2017-02-09T18:31:49Z

flexget/plugins/filter/unique.py

Why is this not done in prepare_config?

cvium · 2017-02-09T18:32:40Z

flexget/plugins/filter/unique.py

Why save it in a variable? I don't anything is gained by it

cvium · 2017-02-09T18:33:21Z

flexget/plugins/filter/unique.py

I'm not a fan of if one-liners. Just move continue down.

cvium · 2017-02-09T18:35:57Z

flexget/plugins/filter/unique.py

What is this?

cvium · 2017-02-09T18:36:36Z

flexget/plugins/filter/unique.py

Nothing happens if field is not specified?

defaults to the entire entity.

def extract_fields(self, entry, field_names): if field_names: pass return {field: entry.get(field) for field in field_names} else: return entry

Should I document it somewhere?

I just wonder if it ever happens that two entries are identical and wouldn't you want to ensure uniqueness on specific parameters? I'm not sure I see the "general" case.

I copied this behavior from the "duplicates" plugin. I think it's a good behavior for ensuring uniqueness of simple entities.
Would you have required specifying field?

The duplicates plugin requires action and field.

OK, I've misremembered.
I'm feeling like if you say "I want these entities to be unique" and you're not saying based on which criteria, it's legit to compare the entire entity.
If you feel like this is a weird behavior, I'll remove it and mandate the field

I think it's better to mirror duplicates plugin and require field.

cvium · 2017-02-09T18:37:19Z

flexget/tests/test_unique.py

cvium · 2017-02-09T18:37:23Z

flexget/tests/test_unique.py

cvium · 2017-02-09T18:37:52Z

flexget/tests/test_unique.py

What are all these imports for?

cvium · 2017-02-09T18:41:16Z

flexget/plugins/filter/unique.py

Isn't any(entry_fields) the same as entry_fields? You can also short-circuit this if by checking entry_fields first ie. if entry_fields and entry_fields == prospect_fields.

BackSlasher · 2017-02-09T18:46:47Z

Thanks for the review! Fixed issues you commented about

cvium · 2017-02-09T18:54:21Z

flexget/tests/test_unique.py

This is not used anywhere

cvium · 2017-02-09T18:55:04Z

flexget/tests/test_unique.py

I don't think this is really needed.

cvium · 2017-02-09T19:03:47Z

flexget/tests/test_unique.py

I don't think this import is used

cvium · 2017-02-09T19:04:06Z

flexget/plugins/filter/unique.py

task.entries only contains entries that are accepted or undecided btw

I think I'd like to keep the skips, in case they're rejected / accepted mid-plugin

cvium · 2017-02-09T19:11:15Z

flexget/plugins/filter/unique.py

Doesn't make sense to check this after you've extracted the fields :)

cvium · 2017-02-09T19:31:04Z

flexget/plugins/filter/unique.py

Won't you potentially end up with a bunch of None values?

I could, but if that's what the user wants to determine uniqueness by...
I'd prefer that than failing the run because of a missing field. Not sure what's the flexget norms on that

I don't think it makes much sense to compare the entries if they don't have all the fields present.

cvium · 2017-02-09T19:32:15Z

flexget/tests/test_unique.py

You don't need these two either.

cvium · 2017-02-09T19:32:53Z

flexget/tests/test_unique.py

This seems wrong :) Beware of copy/pasting. You make a lot of errors.

Yeah. I hope my colleagues will never see this :)

cvium · 2017-02-09T19:35:08Z

flexget/plugins/filter/unique.py

Use https://github.com/Flexget/Flexget/blob/develop/flexget/config_schema.py#L66 instead

Great stuff

I thought this was autoconverting to a list. Apparently not. Oh well

cvium · 2017-02-09T19:50:13Z

flexget/plugins/filter/unique.py

This check is also performed after field extraction

BackSlasher · 2017-02-09T21:15:18Z

No rush whatsoever, but I think I cleared all of the review issues. Would appreciate more feedback!

cvium · 2017-02-10T15:41:45Z

I still think it'd be best to skip entries that do not have all specified fields present.

BackSlasher · 2017-02-10T15:43:42Z

Right now it's failing for these entities, like duplicate:
https://github.com/flexget/Flexget/blob/master/flexget/plugins/filter/duplicates.py#L38
What do you mean "skip"?

BackSlasher · 2017-02-18T12:32:54Z

ping @cvium
also @paranoidi
thanks!

cvium · 2017-02-18T13:15:54Z

flexget/plugins/filter/unique.py

This will crash if the field is not present.

Same here: https://github.com/flexget/Flexget/blob/master/flexget/plugins/filter/duplicates.py#L38
Think we should skip entries/prospects without the required fields instead?

Yes. Duplicates should not be changed in this PR though.

R. Done and added testing for it.

cvium · 2017-02-18T13:47:31Z

flexget/plugins/filter/unique.py

I think it would be better to use EAFP in this case. Just have extract_fields assume all fields are present and catch the KeyError here.

cvium · 2017-02-18T13:57:55Z

flexget/plugins/filter/unique.py

Forgot to change this back?

oh right you are

Takes action on all duplicates, except for the first

cvium · 2017-02-18T14:16:10Z

Hmm, this plugin is starting to look a lot like duplicates. Maybe it would be better to just change duplicates?

Aren't the ideas similar?

BackSlasher · 2017-02-18T14:17:19Z

from #1231:
@paranoidi:

It is semantically very weird to use duplicates plugin to get first unique match. I would make a separate plugin for that purpose which is more suitably named (ie. unique). I was hesitant to even add the action support for this plugin originally. I'm declining this due that concern. I would be willing to accept this refined as a separate plugin though :)

cvium · 2017-02-18T14:20:28Z

I see.

cvium · 2017-02-18T14:22:26Z

Wanna update the wiki?

BackSlasher · 2017-02-18T14:29:08Z

sure, how do I

cvium · 2017-02-18T15:00:39Z

You can pretty much copy https://flexget.com/Plugins/duplicates

https://flexget.com/_create/Plugins/unique

BackSlasher · 2017-02-18T15:02:53Z

Great. Will do soon

BackSlasher · 2017-02-18T15:16:09Z

Done
https://flexget.com/Plugins/unique
@cvium thanks for reviewing this! I appreciate the effort.

cvium requested changes Feb 9, 2017

View reviewed changes

cvium reviewed Feb 9, 2017

View reviewed changes

BackSlasher force-pushed the uniques-plugin branch from 57ba3cd to 356cd14 Compare February 9, 2017 18:46

cvium reviewed Feb 9, 2017

View reviewed changes

flexget/tests/test_unique.py Outdated

Copy link
Copy Markdown

Contributor

cvium Feb 9, 2017

Choose a reason for hiding this comment

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

This is not used anywhere

cvium reviewed Feb 9, 2017

View reviewed changes

flexget/tests/test_unique.py Outdated

Copy link
Copy Markdown

Contributor

cvium Feb 9, 2017

Choose a reason for hiding this comment

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

I don't think this is really needed.

BackSlasher force-pushed the uniques-plugin branch 3 times, most recently from 85a7257 to a19e789 Compare February 9, 2017 19:02

cvium reviewed Feb 9, 2017

View reviewed changes

BackSlasher force-pushed the uniques-plugin branch from a19e789 to d760813 Compare February 9, 2017 19:28

cvium reviewed Feb 9, 2017

View reviewed changes

flexget/tests/test_unique.py Outdated

Copy link
Copy Markdown

Contributor

cvium Feb 9, 2017

Choose a reason for hiding this comment

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

You don't need these two either.

cvium reviewed Feb 9, 2017

View reviewed changes

BackSlasher force-pushed the uniques-plugin branch 3 times, most recently from b0b0f27 to a1b1b37 Compare February 9, 2017 19:49

cvium reviewed Feb 9, 2017

View reviewed changes

flexget/plugins/filter/unique.py Outdated

Copy link
Copy Markdown

Contributor

cvium Feb 9, 2017

Choose a reason for hiding this comment

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

This check is also performed after field extraction

BackSlasher force-pushed the uniques-plugin branch from a1b1b37 to 22c7f90 Compare February 9, 2017 19:51

cvium reviewed Feb 18, 2017

View reviewed changes

BackSlasher force-pushed the uniques-plugin branch from 22c7f90 to 803b728 Compare February 18, 2017 13:43

cvium reviewed Feb 18, 2017

View reviewed changes

BackSlasher force-pushed the uniques-plugin branch from 803b728 to 272b959 Compare February 18, 2017 13:50

cvium reviewed Feb 18, 2017

View reviewed changes

Created unique plugin

dd8b352

Takes action on all duplicates, except for the first

BackSlasher force-pushed the uniques-plugin branch from 272b959 to dd8b352 Compare February 18, 2017 13:58

cvium approved these changes Feb 18, 2017

View reviewed changes

cvium merged commit 77dbb6a into Flexget:develop Feb 18, 2017

Conversation

BackSlasher commented Feb 9, 2017

Motivation for changes:

Detailed changes:

Addressed issues:

Config usage if relevant (new plugin or updated schema):

Log and/or tests output (preferably both):

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BackSlasher Feb 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!

BackSlasher commented Feb 9, 2017

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!

BackSlasher Feb 9, 2017 •

edited

Loading