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

Shpotify rewrite #6

Merged
merged 13 commits into from
Jun 29, 2018
Merged

Shpotify rewrite #6

merged 13 commits into from
Jun 29, 2018

Conversation

dkniffin
Copy link
Contributor

@dkniffin dkniffin commented Apr 27, 2018

Why?

  • As noted in Libspotify no longer available #3, libspotify (previously, a large dependency of this project) was deprecated by Spotify a while back. That left this project pretty much unusable in it's previous version. So, in an effort to revive it, I've redesigned it from the ground up.

What changed?

  • The idea behind this new version of Maestro is very similar to what it was previously, and the usage is similar, but functionally, it's completely different. It is now a Sinatra app that heavily relies on shpotify, a cli tool for interacting with the Spotify desktop client.
  • The majority of this PR is deleting the old code, and setting up a basic Ruby project. It includes:
    • app.rb, which is the actual app code
    • A very minimal Gemfile to install Sinatra and a few dev dependencies
    • Smashing Boxes' standard rubocop configs
    • A readme with instructions for how to use the new app
    • Some very basic tests (more to come in future PRs, hopefully)

Copy link

@jahammo2 jahammo2 left a comment

Choose a reason for hiding this comment

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

How are you deploying this? Just going into a server and running the commands yourself? If so, any plans to set up some Ansible configs (not a criticism as to why they're not here yet, just curious)?

Or am I completely wrong and this is just running locally from one of our abandoned macbooks?

Once you've got a computer to run it on, you can install Maestro by running the following commands
in a terminal:

```sh
Copy link

Choose a reason for hiding this comment

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

what does sh do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It marks it as a shell script for syntax highlighting. Shell == bash for most cases

Copy link

Choose a reason for hiding this comment

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

Nice

Copy link

@jahammo2 jahammo2 left a comment

Choose a reason for hiding this comment

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

This isn't actually working yet, right? It doesn't look there's anything for actually playing music from Spotify. Or am I missing something?

- Autocomplete help text:
- Check "Show this command in the autocomplete list"
- Description: https://github.com/hnarayanan/shpotify
- Usage hint: [shpotify command]
Copy link

Choose a reason for hiding this comment

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

Excellent, excellent documentation

app.rb Outdated
require "sinatra"
require "json"

HELP_TEXT = <<~HELP_TEXT.freeze
Copy link

Choose a reason for hiding this comment

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

What's going on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The <<~HELP_TEXT part is making it a heredoc. The .freeze is because it's a constant, and with our Rubocop configs, we want to enforce that it doesn't change.

Copy link

Choose a reason for hiding this comment

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

👍

app.rb Outdated
puts "# > spotify #{args}"
output = `./spotify.sh #{args}`
puts output
output = HELP_TEXT unless $?.success?
Copy link

Choose a reason for hiding this comment

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

sinatra stuff?

Copy link
Contributor Author

@dkniffin dkniffin May 1, 2018

Choose a reason for hiding this comment

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

Nope. The $? is the return value of the command being run via backticks above (./spotify.sh #{args}). More on that here and it returns a Process::Status, which responds to .success?

Copy link

@jahammo2 jahammo2 May 2, 2018

Choose a reason for hiding this comment

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

How is $? catching that? Or is that just some deep Ruby stuff? Or am I completely wrong and this is somehow shell scripts? At least that's what that stack overflow response seems to indicate. I've never seen shell scripts in a ruby file like this before.

Does adding a ? do anything? Can $ stand alone on its own? I ran echo $ and echo ? from my command line and nothing happened. Well, something happened but just what you'd expect.

Apologies for the question peppering but I always learn a lot from your and this is v fascinating to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is indeed a ruby file. $? automatically gets assigned by the backticks. This might help a bit: https://stackoverflow.com/a/18623297/1202488

? and $ don't do anything on their own. However, I think there are related variables like $#, but I don't remember what they do offhand

app.rb Outdated
end

post "/maestro" do
output, success = spotify(params["text"])
Copy link

Choose a reason for hiding this comment

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

oh that's cool

Copy link

Choose a reason for hiding this comment

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

does output == success return true after they're defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope. This is doing destructuring. So output will equal spotify(params["text"])[0] and success will equal spotify(params["text"])[1]

Copy link

Choose a reason for hiding this comment

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

Ahhhhh, sweet. Is there a reason we haven't done this in any of our Rails apps thus far? Just not knowing that this was even possible is a valid answer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know I've done it before, but I think it just doesn't show up that often. Not sure why

require "rack/test"
require "json"
require "pry-byebug"
require_relative "../app"
Copy link

Choose a reason for hiding this comment

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

I guess this is one of the downfalls of Sinatra, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somewhat. I don't see a huge problem with it, but I might refactor it at some point later. Rails does have a nice way of handling this, and I'm sure we could set it up similarly here.

Copy link

Choose a reason for hiding this comment

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

gotcha

let(:text) { "help" }

it "returns the usage text" do
expect(last_response).to be_ok
Copy link

Choose a reason for hiding this comment

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

Where is last_response coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a rack-test thing: https://github.com/rack-test/rack-test. It's basically the same as response in a normal request spec

Copy link

Choose a reason for hiding this comment

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

And that's available in our Rails apps, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep. It's a dependency of rspec-rails or something

@dkniffin
Copy link
Contributor Author

dkniffin commented May 1, 2018

This is actually working right now. The music playing part happens when we run the shell command via the ./spotify.sh #{args}. That spotify script is shpotify, which is a CLI tool for controlling your local Spotify.app

As for how we're deploying it, right now it's running on a mac mini, and it's all manual. It might be worth writing an ansible script (or maybe just a shell script) to do that, but for now it's manual

@dkniffin
Copy link
Contributor Author

dkniffin commented May 1, 2018

And the reason it's on a mac mini, instead of deployed to heroku or something is that we need to plug speakers into it. The cloud doesn't have a 3.5mm headphone jack, AFAIK :trollface:

@jahammo2
Copy link

jahammo2 commented May 2, 2018

bahahahah, I'll let you know if I hear the Cloud:registered: is adding that feature

* Add Spotify class

* Improve regex capabilities

* Restrict VALID_COMMANDS to public methods
* Implement restart, pause, quit, and toggle

Fix case-sensitivity bug when playing by URI
@dkniffin
Copy link
Contributor Author

Unless anyone sees any issues with it, I'm going to merge this PR around 2:00 this Friday (6/29)

@dkniffin dkniffin merged commit 73c79f3 into master Jun 29, 2018
@dkniffin dkniffin deleted the shpotify-rewrite branch June 29, 2018 18:07
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