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

Modify the blink(1) recipe to allow direct lookup of serial number #10

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

coledot
Copy link

@coledot coledot commented Oct 5, 2014

Handy for assigning a specific blink(1) to a specific task.

@coledot coledot changed the title Modify the thing(1) recipe to allow direct lookup of serial number Modify the blink(1) recipe to allow direct lookup of serial number Oct 5, 2014
@perryn
Copy link
Owner

perryn commented Oct 7, 2014

Hi @coledot

Thanks for the pull request!

A few questions/concerns

  • I'm not sure how the code changes relate to the title of the pull request - all you seem to have done is move where the device is opened? Can you explain further?
  • If you are going to modify tested code, please make sure you update the specs too and make sure they pass
  • There are a number of changes included that just seem to be white space. This makes it difficult to read the pull request. Can you clean that up?

cheers
Perryn

@coledot
Copy link
Author

coledot commented Oct 11, 2014

Hi @perryn,

I modified the Light code to track both the handle and device for a given blink(1), instead of just the handle. Also, in the blink_1.rb recipe, I added a #serial_number method. This method is simply a wrapper around @device.serial_number. This allows one to say light.serial_number to get the blink(1) serial. @handle doesn't provide an equivalent method, from what I can see.

I've fixed the specs as well. The whitespace modifications are to remove excess spaces from the end of lines--just a minor cleanup/annoyance I wanted to deal with.

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