I had a really quick look at this, and the change looks interesting to add in.
However, that's technically speaking a incompatibility change since the result of the count command is different. There should be a document to update in the doc/ directory to explain the change, can you update it? We should also do something about the version number when releasing this, I guess.

Additionally, there're some unit tests in the repository, that would be awesome if you could turn your example into tests!

I can give a more proper look someday next week, if someone else doesn't take this over :)

Alright I'll try to take time to complete and rework the pull request :-)

You could have let this pull request opened and just update your branch as needed, my comment wasn't meant for you to close this and start all over again :) Sorry if you took it this way, that wasn't my intention.

(FYI, doing so allows to keep track of the comments, the modifications, and see what's being worked on.)

On February 23, 2015 11:54:04 PM GMT+08:00, kaliko [email protected] wrote:

Closed #52.

---

Reply to this email directly or view it on GitHub:
#52 (comment)

Humm, sorry. I'm not really used to github process.

I'll reopen the PR, it makes sense indeed to work on this thread of comment.

Thanks for the heads up :-)

I've added the relevant unit test.

I'm not that confident with documentation, I'de rather leave editing to somebody else with better english/reST/sphinx skills.

Changes are rather simple from the API point of view (though the output format changes). Comments on #50 and example provided in the pull request are self explanatory I think.

I've just noticed grouping is available for list command as well (cf. #53).

I believe a better solution can solve both issues, this pull request might be obsolete then.

I'll try to come up with a better solution for count and list.

I believe a way to go would be to expose command's argument in fetch_ methods (retval in _execute method).

It means changing signatures for all _fetch_* methods but it would ease a lot for these cases where MPD answer's format in conditioned by commands arguments.

That's just an idea, I haven't been through all the code and I might have missed some side effects.

Replacing the group argument with a keyword argument looks cleaner to me.

cli.count(group='date') 

The function signatures could look like this:

 def count(self, tag, needle, group=None):
     pass

 def list(self, _type, *args, group=None):
     pass

This has also the advantage, that the group key is known, we do not have to maintain a list of possible group keys.

You mean not going through the mpd._commands abstraction and define explicit methods count/list in MPDClient object? As it's done for noidle for ex.?

I tried to avoid that kind of solutions as it breaks the current design of the module, but I might be wrong, I'm far from a software architect :D

If mpd adds more keyword style arguments, it might be worth to extend the current metaprogramming done with mpd._commands. But currently I think it is easier to just add the methods manually, because the response type changes depending on the arguments, which the current system was not built for.

closed through lack of activity.