Stop displaying the same artist/albums multiple times #181
Conversation
There was a problem hiding this comment.
I do understand why this works... it relies on duplicate artists and albums having the same name and grouping the artist/album IDs together.
However, this makes it impossible to add multiple artists with the same name to a library. Additionally, it may cause false positives with albums (As an example, I'm sure there are multiple artists with albums called "Greatest Hits").
I'm pretty sure the duplicate albums are caused by clean/explicit tracks... but I'm not sure what's causing the duplicate artists - I haven't seen that before.
I'd be willing to accept the album workaround if it also checks the artist... even though I'm not completely sure that would be the best solution. It would be impossible for an artist to have multiple albums with the same name, but it's a much smaller edge case.
Thanks for spending the time on this!
EDIT: It looks like flake8 may have crashed when running in CI, but there are a few linting violations in this PR which I'd like to see fixed before it's merged, if possible.
mopidy_gmusic/library.py
Outdated
| @@ -311,14 +313,30 @@ def sorter(track): | |||
| def refresh(self, uri=None): | |||
| self.tracks = {} | |||
| self.albums = {} | |||
| self.albums2 = {} | |||
There was a problem hiding this comment.
Could this be named something a bit more descriptive? It's always nice to be able to look at a variable and get a bit of context about how it's used.
mopidy_gmusic/library.py
Outdated
|
|
||
| self.albums2[mopidy_track.album.uri] = mopidy_track.album | ||
| album_found = False | ||
| if len(self.albums) > 0: |
There was a problem hiding this comment.
I would just drop this if-else and use the for loop all the time. Looping over an empty list shouldn't be very expensive.
|
I will put in a check for artist / album name to make sure album does not exist |
…bumartist is not converted to lowercase. Previous function would return "Black Sabbath" albums if searching for all albums by "Abba" (because abba is a substring of sabbath)
|
I have done further changes, that's me done unless you want me to do anything else. |
|
I'll try to take another look later. Additionally, could you please make sure flake8 (code style checks) passes for the portions you've changed (It's the part of the travis-ci build that's failing)? There's some information on how to install and run it yourself at http://flake8.pycqa.org/en/latest/. |
|
You know you can run flake8 locally, right? The whole Travis run can be reproduced locally with tox. |
|
Hi Nick,
Thanks for this. I didn't know you could check locally, I should have investigated really. Trying to find that misplaced space has been doing my head in.
Cheers
Jason
…On 10 Nov 2017, 17:55, at 17:55, Nick Steel ***@***.***> wrote:
You know you can run flake8 locally, right? The whole Travis run can be
reproduced locally with tox.
--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#181 (comment)
|
|
I have been there myself. https://docs.mopidy.com/en/latest/devenv/# is really worth doing. It doesn't take that long to setup, it just looks a lot because there's ample helpful explanation around the steps. You might also find |
|
This PR is no longer needed as the issue has been fixed by #233. |
Changes made for issue #139