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

Fix for multiple artist album without album artist tag showing up as an album per artist (issue #254) #258

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ianesten
Copy link

No description provided.

ianesten added 3 commits July 21, 2023 10:26
…an album. this means that the album artist does not have to be set for a compilation to be correctly grouped as one album instead of one album per artist.
@ianesten
Copy link
Author

Note: the database must be recreated for this fix to work.

@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #258 (13cf848) into master (0fa0d55) will decrease coverage by 18.70%.
The diff coverage is 80.00%.

@@             Coverage Diff             @@
##           master     #258       +/-   ##
===========================================
- Coverage   86.29%   67.60%   -18.70%     
===========================================
  Files          46       46               
  Lines        3801     3812       +11     
===========================================
- Hits         3280     2577      -703     
- Misses        521     1235      +714     
Files Changed Coverage Δ
supysonic/scanner.py 94.24% <78.57%> (-1.47%) ⬇️
supysonic/db.py 69.17% <100.00%> (-23.55%) ⬇️

... and 13 files with indirect coverage changes

@spl0k spl0k linked an issue Jul 28, 2023 that may be closed by this pull request
@spl0k
Copy link
Owner

spl0k commented Jul 28, 2023

Hello. As stated in #254, this is not a behaviour I'd like to be introduced in Supysonic. But after thinking a bit about it, I might accept this PR if the behaviour were to be changed a bit. Rather than just picking an album that would reside in the same folder and calling it a day, if this album artist is different than the current track artist I'd rather see the album artist changed to something that clearly states that this is a multi-artist album (such as "Various Artists").

Now, for remarks about the current state of the PR.
First, having to recreate the database is a big no-no. Existing data shouldn't be destroyed. If the schema changes, the you must provide migrations for all providers, both upgrading the schema and the data if needed. But continue reading as this might actually not be necessary.
Secondly, this new folder foreign key in Album doesn't seem useful to me. Tracks have a path that can be used to know in which folder they reside (thing you're already doing). From there you can find other tracks in this folder and then get an album. Tying an album to a folder might not be a good idea as this could lead to some inconsistencies for example if an album is spread across multiple folders (think multi-discs albums where people could store each disc in its own folder). Plus removing this new FK might certainly fix the next remark.
Lastly, make sure the unit tests still pass. Right now a large number of them are failing because of the new FK. You can run them using the command

python -m unittest

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.

Multiple artist albums have only 1 track
2 participants