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

Duplicates plugin: tiebreak album vs items fix #2758

Merged
merged 2 commits into from
Dec 12, 2017
Merged

Conversation

cgevans
Copy link
Contributor

@cgevans cgevans commented Dec 10, 2017

At present, if tiebreak is specified to give keys for the order of priority in determining duplicates, it must be specified for both items and albums, or an error will be raised when the unspecified one is used. This is particularly bad because the default order of priority appears to be impossible to specify in tiebreak, so if the user wants (as in my case) a tiebreak specification for items, and the default order for albums, this is impossible.

This small change makes it so that tiebreak is only used if it specifies keys for ordering the kind (track/album) being considered; otherwise, the default is used.

…nsidered, otherwise use default (eg, if only items is specified, and -a is being used, use default order of priority
Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

Thanks for catching this! It does indeed look like a problem.

It seems like there may be another potential problem with calling all_keys on kind if that variable is now going to be a string instead of a class object. Maybe we should come up with a separate variable name for that?

Also, would you mind adding a brief changelog entry describing the problem that was fixed?

key = lambda x: tuple(getattr(x, k) for k in tiebreak[kind])
else:
kind = Item if all(isinstance(o, Item) for o in objs) else Album
if kind is Item:
if kind is 'items':
Copy link
Member

Choose a reason for hiding this comment

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

I think the proper comparison operator on strings is ==, not is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is... oops.

key = lambda x: tuple(getattr(x, k) for k in tiebreak[kind])
else:
kind = Item if all(isinstance(o, Item) for o in objs) else Album
Copy link
Member

Choose a reason for hiding this comment

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

Line 269, below, assumes that kind is a Model class, not a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops... that code is never run in my specific configuration, but line 269 just needs Item.all_keys(), so it's an easy change.

@cgevans
Copy link
Contributor Author

cgevans commented Dec 11, 2017

Those issues should be fixed now, thanks.

@sampsyo
Copy link
Member

sampsyo commented Dec 12, 2017

Perfect; thank you! ✨

@sampsyo sampsyo merged commit dd2b44e into beetbox:master Dec 12, 2017
sampsyo added a commit that referenced this pull request Dec 12, 2017
Duplicates plugin: tiebreak album vs items fix
sampsyo added a commit that referenced this pull request Dec 12, 2017
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.

2 participants