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

mpdsync.py: Remove "file: " prefix from song string in full_sync(). #1

Closed
wants to merge 0 commits into from

Conversation

alphapapa
Copy link

Before this change, I was getting this traceback:

Traceback (most recent call last):
  File "mpdsync.py", line 208, in <module>
    main()
  File "mpdsync.py", line 80, in main
    full_sync(master, slaves)
  File "mpdsync.py", line 176, in full_sync
    slave.client.add(song)
  File "/usr/lib/python2.7/dist-packages/mpd.py", line 167, in <lambda>
    return lambda *args: wrapper(command, args)
  File "/usr/lib/python2.7/dist-packages/mpd.py", line 213, in _execute
    return retval()
  File "/usr/lib/python2.7/dist-packages/mpd.py", line 312, in _fetch_nothing
    line = self._read_line()
  File "/usr/lib/python2.7/dist-packages/mpd.py", line 233, in _read_line
    raise CommandError(error)
mpd.CommandError: [50@0] {add} Not found

Now it works with mpd 0.18.7 and 0.19.1 on Ubuntu 14.04 and Debian Jessie.

@alphapapa
Copy link
Author

Also moved config file to ~/.config/mpdsync.json and used XDG module to find $XDG_CONFIG_HOME.

@alphapapa
Copy link
Author

Also changed shebang to use /usr/bin/env so the script can be executed properly.

@@ -91,9 +94,9 @@ def main():
sync(master, slaves)


def get_settings(settings_file="settings.json"):
def get_settings(settings_file=os.path.join(BaseDirectory.xdg_config_home, "mpdsync.json")):
Copy link
Owner

Choose a reason for hiding this comment

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

Since I prefer to use stdlib as much as possible, could you use os.path.expanduser() here instead?

Something like this:

def get_settings(settings_file=os.path.expanduser('~/.config/mpdsync.json')):

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I wondered if it was a good idea to depend on python-xdg, and if it was really necessary. I doubt that many people actually change $XDG_CONFIG_HOME to something other than ~/.config. On the other hand, python-xdg looks to be a fairly standard package (especially on Ubuntu, where software-center depends on it, so it should be installed by default). And it seems like the more "correct" way to do it, rather than hard-coding paths.

But I don't really care very much; I use ~/.config anyway. I suppose the best reason might be to make it easier for potential Windows users, but I don't care much about supporting Windows either, haha!

So, yeah, if you prefer doing it that way, that's fine with me. Thanks.

@nickpegg
Copy link
Owner

nickpegg commented Feb 5, 2015

Thanks for the /usr/bin/env thing and cleaning up my whitespace! Pretty sure /usr/bin/which didn't work and was a brainfart on my part when I wrote this script long ago.

@nickpegg nickpegg mentioned this pull request Feb 5, 2015
@alphapapa
Copy link
Author

The whitespace fix there was either an accident or a result of using Emacs, but I'm glad it helped. =)

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.

2 participants