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

Finally started to get somewhere :-) #1

Closed
wants to merge 7 commits into from

Conversation

nateroling
Copy link

I finally got around to cleaning up the ImportTest I wrote.

I haven't merged your latest commit, because I'm not 100% sure the best way to handle that. I don't want to do it as a rebase, I don't think. I imagine I just want to merge your changes in. Let me know if you want me to do that before you pull my commits in. I'm sure GitHub has a very nice tutorial on it somewhere that I can read. I should get around to reading the Git Book at some point.

Anyway, let me know what you think.

Ozzi Lee added 7 commits February 2, 2011 13:08
Tracks could be deleted when re-importing tracks that are already in
the library directory and import_delete is set to yes.
BaseAlbum needs to use object.setattr in its __init__ method, because
it overrides __setattr__.
test_library.py so far just documents existing behavior of album
objects, especially the fact that trying to access unset fields causes
an AttributeError.
If the track.albumartist isn't set on the first track, then the
album.albumartist is set to the track.artist.
Only tests "as-is" metadata imports, making sure that tracks are
copied to the library and named properly.
@nateroling
Copy link
Author

I went ahead and merged in your last commit. It seems like the right thing to do:
http://help.github.com/forking/

@sampsyo
Copy link
Member

sampsyo commented Feb 18, 2011

Hey, thanks! This test looks great. I'm glad someone's finally gotten around to writing good integration tests for the importer -- that's good peace of mind.

I'm pushing your changes to the main repository with a couple of changes.

  • Moved tests from test_album to test_db (alongside the rest of the tests for albums).
  • Changed some things about how artists (and now artist IDs) are "guessed" from files that don't have them. This behavior is, for example, only enabled for "as-is" imports.
  • Reorganize the test class a little bit for reusability.
  • I also added the ability to disable the "progress" (.beetsstate file) functionality for testing.

Let me know if I messed anything up. Thanks again for your help!

(BTW, in the future, I also don't mind doing the merge on my end when I pull in your changes.)

@nateroling
Copy link
Author

It all looks golden to me :-)

@iluvdirt iluvdirt mentioned this pull request Jan 9, 2014
sampsyo added a commit that referenced this pull request Mar 22, 2014
This is, shockingly, the #1 question I get asked. I hope this helps.
sampsyo added a commit that referenced this pull request Sep 30, 2014
This was well-intentioned but ended up being more confusing than it was worth.
It's always confused me when one digit gets un-highlighted in one of these
displays. The straw that broke the camel's back was when I got a "#1 -> #16"
change where the numeral "1" was un-highlighted. To fix this right would be
way more trouble than it's worth; I'm glad to be rid of this detail.
sampsyo added a commit that referenced this pull request Oct 4, 2014
This was well-intentioned but ended up being more confusing than it was worth.
It's always confused me when one digit gets un-highlighted in one of these
displays. The straw that broke the camel's back was when I got a "#1 -> #16"
change where the numeral "1" was un-highlighted. To fix this right would be
way more trouble than it's worth; I'm glad to be rid of this detail.

Conflicts:
	docs/changelog.rst
sampsyo pushed a commit that referenced this pull request Mar 7, 2018
@hrehfeld hrehfeld mentioned this pull request Aug 19, 2018
wisp3rwind added a commit to wisp3rwind/beets that referenced this pull request Aug 19, 2019
fixes issue beetbox#3346: For (hidden) tracks with index 0, the UI would
previously show a #0 -> beetbox#1 change when actually the index would be set to 0.
Now, properly distinguish index 0 and None (i.e. not set)
wisp3rwind added a commit to wisp3rwind/beets that referenced this pull request Aug 19, 2019
fixes issue beetbox#3346: When the per_disc_numbering option was set, the UI would
previously show a #0 -> beetbox#1 change when actually the index would be set
to 0 (a valid index, such as for hidden tracks).  Now, properly
distinguish index 0 and None (i.e. not set)
wisp3rwind added a commit to wisp3rwind/beets that referenced this pull request Aug 19, 2019
fixes issue beetbox#3346: When the per_disc_numbering option was set, the UI would
previously show a #0 -> beetbox#1 change when actually the index would be set
to 0 (a valid index, such as for hidden tracks).  Now, properly
distinguish index 0 and None (i.e. not set)
JOJ0 pushed a commit that referenced this pull request Dec 17, 2023
Sort and categorize config_default.yaml
snejus added a commit that referenced this pull request Nov 23, 2024
Fixes #5148. 

When importing, the code that matches tracks does not consider the
medium number. This causes problems on Hybrid SACDs (and other releases)
where the artists, track numbers, titles, and lengths are the same on
both layers.

I added a distance penalty for mismatching medium numbers.

Before:

```
$ beet imp .

/Volumes/Music/ti/Red Garland/1958 - All Mornin' Long - 1 (6 items)

  Match (95.4%):
  The Red Garland Quintet - All Mornin' Long
  ≠ media, year
  MusicBrainz, 2xHybrid SACD (CD layer), 2013, US, Analogue Productions, CPRJ 7130 SA, mono
  https://musicbrainz.org/release/6a584522-58ea-470b-81fb-e60e5cd7b21e
  * Artist: The Red Garland Quintet
  * Album: All Mornin' Long
  * Hybrid SACD (CD layer) 1
     ≠ (#2-1) All Mornin' Long (20:21) -> (#1-1) All Mornin' Long (20:21)
     ≠ (#2-2) They Can't Take That Away From Me (10:24) -> (#1-2) They Can't Take That Away From Me (10:27)
     ≠ (#2-3) Our Delight (6:23) -> (#1-3) Our Delight (6:23)
  * Hybrid SACD (CD layer) 2
     ≠ (#1-1) All mornin' long (20:21) -> (#2-1) All Mornin' Long (20:21)
     ≠ (#1-2) They can't take that away from me (10:27) -> (#2-2) They Can't Take That Away From Me (10:25)
     ≠ (#1-3) Our delight (6:23) -> (#2-3) Our Delight (6:23)
➜ [A]pply, More candidates, Skip, Use as-is, as Tracks, Group albums,
Enter search, enter Id, aBort, eDit, edit Candidates?
```

Note that all tracks tagged with disc 1 get moved to disc 2 and vice
versa.

After:

```
$ beet-test imp .

/Volumes/Music/ti/Red Garland/1958 - All Mornin' Long - 1 (6 items)

  Match (95.4%):
  The Red Garland Quintet - All Mornin' Long
  ≠ media, year
  MusicBrainz, 2xMedia, 2013, US, Analogue Productions, CPRJ 7130 SA, mono
  https://musicbrainz.org/release/6a584522-58ea-470b-81fb-e60e5cd7b21e
  * Artist: The Red Garland Quintet
  * Album: All Mornin' Long
  * Hybrid SACD (CD layer) 1
     ≠ (#1-1) All mornin' long (20:21) -> (#1-1) All Mornin' Long (20:21)
     ≠ (#1-2) They can't take that away from me (10:27) -> (#1-2) They Can't Take That Away From Me (10:27)
     ≠ (#1-3) Our delight (6:23) -> (#1-3) Our Delight (6:23)
  * Hybrid SACD (SACD layer) 2
     * (#2-1) All Mornin' Long (20:21)
     * (#2-2) They Can't Take That Away From Me (10:24)
     * (#2-3) Our Delight (6:23)
➜ [A]pply, More candidates, Skip, Use as-is, as Tracks, Group albums,
Enter search, enter Id, aBort, eDit, edit Candidates?
```

Yay!
This pull request was closed.
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