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

Abstract client #39

Merged
merged 18 commits into from Dec 2, 2011
Merged

Abstract client #39

merged 18 commits into from Dec 2, 2011

Conversation

medwards
Copy link
Contributor

@medwards medwards commented Nov 24, 2011

We take Client and make it a class to be inherited by real clients. The setup script will select afplay or mpg123 based on what it can find, we also add support for mpc (the mpd client) since that is what we use at our workplace.

@holman
Copy link
Member

@holman holman commented Dec 2, 2011

This looks pretty good.

There's been a few changes since this Pull came in; would you mind merging master in and resolving any conflicts?

A few more bits of feedback I'd like to tackle before this gets merged properly:

  • Can you namespace clients under lib/play/clients? It's just a lot clearer, particularly if we end up with a number of clients.

Okay maybe that's it. Looks pretty sharp.

@medwards
Copy link
Contributor Author

@medwards medwards commented Dec 2, 2011

So, I have some concerns with testing of clients. I'd like to make TestClient more realistic and then provide a way to compare the operation of TestClient with an arbitrary client implementation which should help client maintainers keep their code quality up.

But thats another day. Today :shipit: :D

@@ -104,6 +117,7 @@ sed -i '' "s/__OAUTH_KEY__/$client_id/" config/play.yml
sed -i '' "s/__OAUTH_SECRET__/$client_secret/" config/play.yml
sed -i '' "s/__USER__/$user/" config/play.yml
sed -i '' "s/__PASSWORD__/$password/" config/play.yml
sed -i '' "s/__CLIENT__/$client/" config/play/yml
Copy link
Contributor

@nandub nandub Dec 2, 2011

Choose a reason for hiding this comment

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

I think you meant config/play.yml and not config/play/yml

@medwards
Copy link
Contributor Author

@medwards medwards commented Dec 2, 2011

blurgh. Thanks for the catch.

@holman holman mentioned this pull request Dec 2, 2011
@holman holman merged commit 10117fe into play:master Dec 2, 2011
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.

None yet

3 participants