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

bpd: support short form of list command for albums #3215

Merged
merged 2 commits into from Apr 15, 2019

Conversation

Projects
None yet
2 participants
@arcresu
Copy link
Member

commented Apr 13, 2019

Some clients list the albums belonging to an artist by issuing the command list album <ARTIST NAME>. This change inserts the tag artist before the artist name so that this succeeds. Fixes #3007.

I confirmed the behaviour with the current MPD. It gave a rather helpful error message ACK [2@0] {list} should be "Album" for 3 arguments that shows that the shorter version is only for getting the albums of an artist.

A small unrelated change is to address a minor issue where if clients listed all album artists the first result would be blank (due to not all items in beets' database having album artist tags). We just skip any blank responses for the specific case of listing all unique values of a tag.

arcresu added a commit to arcresu/beets that referenced this pull request Apr 13, 2019

@sampsyo
Copy link
Member

left a comment

Looks great! Thanks for making both of these changes. I just left one comment inline about mentioning that this seems to be a nonstandard extension…

Show resolved Hide resolved beetsplug/bpd/__init__.py Outdated

arcresu added some commits Apr 13, 2019

bpd: support short form of list command for albums
Some clients list the albums belonging to an artist by issuing the
command `list album <ARTIST NAME>`. This change inserts the tag `artist`
before the artist name so that this succeeds. Fixes #3007

@arcresu arcresu force-pushed the arcresu:bpd-compat branch from 98b4c7b to 7ddde2a Apr 15, 2019

@arcresu

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

Just to note though that the MPD developer has indicated that the MPD protocol docs are not complete and that the software should be used as a reference to answer any questions. I don't think this is non-standard behaviour, just an undocumented part of the protocol that clients were already relying on.

The MPD query syntax was overhauled in 0.21, so I think this syntax is only still supported by MPD for backwards-compatibility. It might even be that the clients stop issuing these type of queries once we start claiming to be a more modern version of MPD.

@arcresu arcresu merged commit 7b910c3 into beetbox:master Apr 15, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@arcresu arcresu deleted the arcresu:bpd-compat branch Apr 15, 2019

@arcresu arcresu referenced this pull request Apr 15, 2019

Merged

bpd: support MPD 0.16 protocol and more clients #3214

15 of 20 tasks complete
@sampsyo

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

Got it; that makes sense! In that case, I suppose it's not shocking that the real-world usage would differ from the docs. Thanks for clarifying.

@arcresu arcresu added this to the Modern MPD support milestone Jun 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.