vimus / libmpd-haskell

A client library for MPD, the Music Player Daemon
MIT License
36 stars 26 forks source link

Support MPD 0.21 style filters #129

Closed iyefrat closed 3 years ago

iyefrat commented 3 years ago

In version 0.21 MPD added a more expressive syntax for filters. This PR implements it in libmpd in a backwards compatible way by changing the Query type from

data Query = Query [Match]

to:

data Query = Query [Match] | Filter Expr

where Expr corrisponds to the newer syntax. This change is backwards compatible, both in the sense that it doesn't break existing Haskell code using libmpd (tests pass, vimus compiles), and that libmpd generates MPD 0.20 style filters for queries not created with the new combinators.

The new combinators are:

Notice that this doesn't implement the AudioFormat filters, because I'm not entirely sure how those work. At any rate like the rest of this stuff they can be implemented without breaking backwards compatibility because the user only creates Query's with the combinators anyway.

I haven't created tests for these yet, primarily because I haven't really been able to understand the test suite all that well. For example, why does this work? The default song doesn't have a title.

Edit: also, this closes #77, and is related to #123.

iyefrat commented 3 years ago

@psibi @joachifm @sol ?

psibi commented 3 years ago

@iyefrat Sorry for the late reply. I don't have any oppositions in merging this. I'm slightly concerned that there are no tests for this. Is it possible to add minimal tests for the newly added functions ?

iyefrat commented 3 years ago

Yes if I wasn't clear, I want to add tests before this gets merged, it's just that I'm having trouble understanding how the song portion of the test suite works. For example this test seems to query for a song with title "Foo" on a database consisting of a song with no title, but it returns that song anyway. Can you explain to me what I'm missing here?

psibi commented 3 years ago

@iyefrat To be honest, even I don't have much idea about it. Probably someone else can chime in and answer that.

But probably adding a new test module can be added for the new functions ?

iyefrat commented 3 years ago

@psibi It's just that the part of the test suite I'm asking about is the one about =? and all the functions are generalizations of that so I don't really think I can write the tests for the new functions until I understand that

joachifm commented 3 years ago

Re: the test. It's been a long while, but at a glance it only exercises the parsing, it does not actually evaulate/simulate the query.

psibi commented 3 years ago

@iyefrat Does the clarification from Joachifm help you ? Let me know if I should look further into it and I can likely get some time to do it tomorrow.

iyefrat commented 3 years ago

@psibi It kind of did. I went over the test code again and it does seem to just make sure that the command parses correctly, but now I'm just confused as to why it was designed this way, as the whole fake song part of this seems to be entirely redundant. Maybe to lay the groundwork for testing on an actual song library in the future?

Either way, I have implemented the tests. Because they only do parsing, they still rely on me having written the results correctly. I have checked that all the new queries actually work on my end, and this is the existing testing standard anyway.

psibi commented 3 years ago

Thank you!