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

added listallinfo command #72

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

paulchambaz
Copy link

For a personal project of mine, I needed to have quick access to mpd "listallinfo" command.
Since the mpd "listall" command was already implemented, and a Vec structure could already be built from the mpd response, I added the mpd "listallinfo" command. After testing the new command in my project, I had the behaviour I expected.
Let me know if I need to add more code for the pr to be valid.

Copy link
Contributor

@naglis naglis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

  1. listallinfo (like listall) lists directories, not just songs. Currently, the listall() method is incomplete/incorrect (it does not handle directories, see also issue listall() output is broken #64).
  2. This comment should be altered to remove it out of TODO.
  3. A test would be appreciated.

src/client.rs Outdated
@@ -203,6 +203,11 @@ impl<S: Read + Write> Client<S> {
self.run_command("listall", ()).and_then(|_| self.read_structs("file"))
}

/// lists all songs in the database with metadata
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// lists all songs in the database with metadata
/// Lists all songs in the database with metadata

@paulchambaz
Copy link
Author

  1. I'm sorry, but I do not think I am competent enough to add the uri myself. Since listall is incomplete but also present, maybe we could add listallinfo since once one will be fixed, the other will be easily fixed to?
  2. I have changed the TODO and will update the pr
  3. Again, I'm sorry, I am very new to doing pr, i tried looking for a test on listall to try to test something similar, but I could not find any. Due to the nature of the function, a function that queries the mpd database directly, I'm unsure on the kind of test I could write that would test correct behaviour. It seems to me that both run_command and read_structs should be tested, but I'm afraid I do not have the skill to do such a test.

I have corrected the incorrect docstring and removed the TODO. Again, I'm sorry I cannot be more of use for this project.

@eshrh
Copy link

eshrh commented Aug 27, 2024

Hi, just to let you know, I thought your patch was very useful, so I've included it in my fork (i just copied the exact patch from the PR). I hope you don't mind!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants