search

andrewrk / mpd.js

Connect to a music player daemon server, send commands, emit events.
MIT License
89 stars 18 forks source link

Added support for arrays in parseKeyValueMessage. #12

Closed nmunro closed 10 years ago

nmunro commented 10 years ago

Hey, I found that things like 'playlistinfo' ended up only returning one song object when a playlist would likely contain more than one song.

Hope you find it to be useful.

andrewrk commented 10 years ago

I think I'd rather have 2 different functions, one that is meant to parse a single object, and one that is meant to parse multiple objects. So the return value would be consistent; one function would always return an object, and the other would always return an array. And you have to know which one to call.

nmunro commented 10 years ago

I can address that, I can't think of a situation when there's some ambiguity as to what you'd want. Can you? Obviously it should be as clear as possible in each situation.

andrewrk commented 10 years ago

If there's no ambiguity then it's not inconvenient to have to call the correct function. I think it's a good technique to avoid errors to have function parameters and return values always be the same type.

nmunro commented 10 years ago

I've created a new patch, ought I close this pull request and create a new one?

andrewrk commented 10 years ago

You can do that, or you can force push which will update this pulll request.

nmunro commented 10 years ago

Which is better for you?

andrewrk commented 10 years ago

Either way is fine. If I were doing it I'd just force push.

underr commented 9 years ago

It would be nice to have the parseArrayMessage function on README.md, I had to read the source code to find it.