search

jwhitehorn / pi_piper

Event driven Raspberry Pi GPIO programming in Ruby
BSD 2-Clause "Simplified" License
694 stars 71 forks source link

Move to gem-drivers logic #66

Closed elmatou closed 8 years ago

elmatou commented 8 years ago

Create PiPiper::Driver (skeleton for gemed drivers & shell for specs) Remove bcm2835.rb to be found later in bcm2835 & sysfs gems remove libbcm2835.so to specific gem move pin_values.rb to drivers or objects that needs them move pin_error to PiPiper module move & update Platform features to PiPiper module spec some PiPiper features (need still some work) update Pin, Pwm, Spi, I2C to work with gemed driver logic Pin: move #pull! to initialize not an instance method anymore (to be discussed) also moved validation to driver level (nothing in PiPiper::Driver)

I'll test and push the to first drivers the weekend.

see you

zsyed91 commented 8 years ago

can you rebase your branch from dev? I branched off that one and it seems like your changes are more than what you did for this PR :smile:

It seems like the travis builds are failing because some of the Gemfile changes are missing I guess. I think rebasing should fix that.

From an initial glance though, the code seems reasonable.

elmatou commented 8 years ago

I look at it later in the day. I'll push too my on going work on pi_piper-sysfs and pi-piper-bcm2835

El Matou

On Mon, Mar 7, 2016 at 6:32 AM, Zshawn Syed notifications@github.com wrote:

can you rebase your branch from dev? I branched off that one and it seems like your changes are more than what you did for this PR [image: :smile:]

— Reply to this email directly or view it on GitHub https://github.com/jwhitehorn/pi_piper/pull/66#issuecomment-193110839.

elmatou commented 8 years ago

I can't understand why travis fails...

You are trying to install in deployment mode after changing
your Gemfile. Run `bundle install` elsewhere and add the
updated Gemfile.lock to version control.

but Gemfile.lock is versioned (even if it shouldn't...)...

or removing it will fix it !?

elmatou commented 8 years ago

I don't understand why do you want to rebase the branch. It seems to be in sync with dev & master. at least in @jwhitehorn repos.

zsyed91 commented 8 years ago

Ah you are right, I forgot to rebase the new gem_structure_changes branch after we merged some changes. No worries.

Can you run bundle install and then see if the Gemfile.lock changed? If it did, go ahead and commit it. That should fix those issues.

And better yet, maybe squash all the commits so each PR is a single commit that gets merged. I like how that approach keeps a clean history. I have done it on other projects and noticed that quite a few other projects use this approach. I will try to be mindful of squashing my commits now as well. Once we get this branch ready to merge, please be prepared to squash all the commits :smile:

elmatou commented 8 years ago

Isn't a best practice not to version gemfile.lock for gems ? On Mar 7, 2016 7:12 PM, "Zshawn Syed" notifications@github.com wrote:

Ah you are right, I forgot to rebase the new gem_structure_changes branch after we merged some changes. No worries.

Can you run bundle install and then see if the Gemfile.lock changed? If it did, go ahead and commit it. That should fix those issues.

— Reply to this email directly or view it on GitHub https://github.com/jwhitehorn/pi_piper/pull/66#issuecomment-193375587.

zsyed91 commented 8 years ago

I think so but travis relies on the lockfile so I suppose we have to.

zsyed91 commented 8 years ago

hooray tests are passing! :smile:

zsyed91 commented 8 years ago

Looks good overall. There are just a couple minor issues I commented on.

zsyed91 commented 8 years ago

@elmatou One minor comment I put in the code. And regarding the tests, I don't think its conventional to put the arguments in the it statement. It should be it '#do_something' instead of it '#do_something(x, y)'. Also the it should really be a describe.

elmatou commented 8 years ago

I corrected the ArgumentError thing, which makes total sense. I disagree on the describe/it argument. Anyhow driver_spec.rb only intend to fix the API.

zsyed91 commented 8 years ago

Cool I will take a look shortly.

Please take a look at this: http://betterspecs.org/ and https://relishapp.com/rspec/rspec-core/v/3-3/docs/example-groups/basic-structure-describe-it. Convention exists to help others get on board and keeps everything more maintainable.

zsyed91 commented 8 years ago

@elmatou :+1: Very nice work. Thank you for updating the Pin spec. It is definitely much better now. Do you have anything else you need/want to commit in this PR? If not, go ahead and squash everything to 1 commit and I can merge it.

elmatou commented 8 years ago

sqashed !

zsyed91 commented 8 years ago

looks good to merge