-
Notifications
You must be signed in to change notification settings - Fork 38
Add some more commands. #21
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
Conversation
The failure is due to https://github.com/Manishearth/rust-clippy/issues/1476 |
} | ||
|
||
/// Find all songs in the db that match query and adds them to current playlist. | ||
pub fn findadd(&mut self, query: &Query) -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought of joining up searchadd
and findadd
together into a single method, as the functionality of these commands are essentially the same, except for case-sensitivity, which can be passed either as a separate parameter, or as a field in Query
object (debatable).
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On one hand, having something like that would make sense. On the other, keeping them separate makes it easier for somebody looking at MPD documentation to figure out what the corresponding method is. Given that most of the methods currently do match with the underlying MPD commands, I'd be inclined to at least have a lower-level interface that more closely mirrors the underlying protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll merge it as is for now, but I think this point should be thought through more thoroughly.
5419e60
to
8c5eafd
Compare
8c5eafd
to
8674770
Compare
No description provided.