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

Merge in importer. #1535

Closed
wants to merge 6 commits into from
Closed

Merge in importer. #1535

wants to merge 6 commits into from

Conversation

DArtagan
Copy link

@DArtagan DArtagan commented Jul 6, 2015

When duplicate records are found during import, the user has the option to merge the two sets together. Presently, only works when tracks don't conflict.

This is not the final commit, I'm planning on resolving track conflicts before this is ready for merging. Before I head down that road though, I'd really like my work double-checked (@sampsyo). This project is impressively large, I did my best to understand this portion, but I'm not sure if I'm abusing the database or pipeline or something.

Cheers!

P.S. Related to issue #112

When duplicate records are found during import, the user has the option to merge the two sets together.  Presently, only works when tracks don't conlict.
@DArtagan
Copy link
Author

@sampsyo a few questions:

  • In for the unittests, I wanted to test merging against an album with multiple tracks. Making a list of a couple TrackInfo objects only seems to ever have the first one picked up, why?
  • I'd like to also allow for interactive merging, where two albums contain overlapping parts of each other. My first thought is to compare the track ids, would you agree with this approach? Would mocks be the proper way to unittest such interactions?
  • I'm not sure why the builds are failing on test.test_mb.MBLibraryTest.test_match_album. It built fine when I committed last weekend, but not this weekend? All 37 tests in test_mb.py pass on my machine.
  • I see you're running AppVeyor in addition to Travis CI. What do you think of it?

@sampsyo
Copy link
Member

sampsyo commented Aug 3, 2015

Hi! Thank you for writing this! It looks amazing, and it fulfills something that many people have asked for. And I'm very very sorry that I didn't get around to this earlier—thanks for your patience while I caught up on the backlog.

I'll review the code now. To answer your questions:

In for the unittests, I wanted to test merging against an album with multiple tracks. Making a list of a couple TrackInfo objects only seems to ever have the first one picked up, why?

I'm not 100% sure what's going on here—are you referring to the new test_merge_duplicate_album_without_duplicate_tracks, but with the commented-out lines commented back in? If so, should I just run that to see what you're talking about?

I'd like to also allow for interactive merging, where two albums contain overlapping parts of each other. My first thought is to compare the track ids, would you agree with this approach?

Is the idea to let you break apart an album and then merge part of it into a new album? That sounds like a rather complex thing to orchestrate. Would you mind terribly much if we left that extension until after we finish this more basic version?

Would mocks be the proper way to unittest such interactions?

Probably—depending on where the user input comes in.

I'm not sure why the builds are failing on test.test_mb.MBLibraryTest.test_match_album. It built fine when I committed last weekend, but not this weekend? All 37 tests in test_mb.py pass on my machine.

That's not your fault, and now fixed. Nothing to worry about!

I see you're running AppVeyor in addition to Travis CI. What do you think of it?

So far so good; it seems to do its job admirably, and it's great that it's free for open source. It's disabled for us, unfortunately, because we haven't gotten all the tests passing on Windows quite yet (#670).

@@ -810,6 +810,10 @@ def resolve_duplicate(self, task, found_duplicates):
elif sel == 'k':
# Keep both. Do nothing; leave the choice intact.
pass
elif sel == 'm':
# Merge the two together
Copy link
Member

Choose a reason for hiding this comment

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

Style nitpick: for comments like this, please use a period at the ends of sentences.

@DArtagan
Copy link
Author

DArtagan commented Feb 9, 2018

Mad props to udibox1209, making this happen in #2725.

@DArtagan DArtagan closed this Feb 9, 2018
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