getState now includes track id for easier polling

andrehaveman / spotify-node-applescript

Control Spotify on Mac OSX with NodeJS and AppleScript

MIT License

316 stars 38 forks source link

getState now includes track id for easier polling #11

Closed rmehner closed 11 years ago

rmehner commented 11 years ago

track_id is now part of the JSON of getState. This commit also extracts getState out to a seperate file, making maintainance eaiser.

rmehner commented 11 years ago

Two things:

I added track_id into the state output as it makes it way easier to poll for track changes which is in turn needed if you want to build something like a remote for Spotify, as you don't want to poll for the track regularly. If you don't agree with that change I'm happily removing that line and just do the seperate file thing here.
As soon as this and #10 is merged, I'd love to do a small refactoring to make this thing even more readable (e.g. the execFile is the same for all three seperate file things, so that can be abstracted like the execScript method), but I didn't want to do it right now, as this eases merging for you.

andrehaveman commented 11 years ago

Merged it.

About the suggested refactoring: I had the same idea last weekend, and did a similar refactoring in this branch: https://github.com/andrehaveman/spotify-node-applescript/blob/separate_applescript_files/lib/spotify-node-applescript.js. Please let me know what you think about it.

rmehner commented 11 years ago

Just looked at your refactoring. This is nearly the same as I had in mind, so +1 on that.

Things I'd have done different:

I would have separated execFile and execScript to separate methods that the global methods use. But guess that's a matter of taste. I just would have liked it, as it embraces the "if it's more than one line, separate that to an extra file" a bit more IMHO. Anyway, no biggie.
2 Spaces! ;-)

Would be good to have some new releases then, makes it easier to depend on it, as NPM is having some issues with Git dependencies

Anything I can help with?

andrehaveman commented 11 years ago

I wanted the choice for string or file in one place (the scripts object). That's why I chose to incorporate file execution in the execScript function.

I also replaced the transformer function with an extra param for the execScript function and use util.transform instead.

I will release it when I have merged the last open pull request (in a few days).

rmehner commented 11 years ago

Everything very good, thank you for your work and as said: let me know when I can help.

rmehner commented 11 years ago

On a second note: I'm happy to provide the tests for the mute/unmute thing if that's a blocker. I just can't fork his repo at the moment, because that would eliminate my fork on which spotify-remote depends on.

andrehaveman commented 11 years ago

Thanks, but already wrote the test yersterday evening :). I also did a small change in getState. The position is now returned as number instead of string. I will release a new version this evening.