openbci-archive / OpenBCI_NodeJS

Node.js SDK for the all OpenBCI Biosensor Boards
https://www.npmjs.com/package/openbci
138 stars 50 forks source link

Enh semi standard and others #88

Closed andrewjaykeller closed 8 years ago

andrewjaykeller commented 8 years ago

1.4.0

New Features

codecov-io commented 8 years ago

Current coverage is 93.33% (diff: 95.27%)

Merging #88 into master will increase coverage by 0.64%

@@             master        #88   diff @@
==========================================
  Files             4          4          
  Lines          2186       2174    -12   
  Methods         128        126     -2   
  Messages          0          0          
  Branches        478        476     -2   
==========================================
+ Hits           2026       2029     +3   
+ Misses          160        145    -15   
  Partials          0          0          

Powered by Codecov. Last update 6fd9ddf...a6eb7e2

andrewjaykeller commented 8 years ago

@baffo32 what are your thoughts on this PR in regards to #82 and your work on it? I can remove the semi standard adoption from this PR if it will mess up your PR too much.

baffo32 commented 8 years ago

wow !

Maybe I should redo #82 on top of this. It's a pretty small change, conceptually, and this looks huge at first glance. Might be less work than trying to merge them.

baffo32 commented 8 years ago

are you automatically applying semi-standard with a tool, such that I could make the same automatic changes to ease merging, or manually handling each of the complaints of the linter?

andrewjaykeller commented 8 years ago

Hey! Yes i am using a standard tool that will check to make sure everything is listing properly called semi-standard

By simply running the command semistandard we are able to verify everything is listing correctly.

I'm not sure what to do here... my gut says:

  1. Remove semi-standard from this PR and merge in the small patch to bump to v1.3.3
  2. You rebase off of that and continue building with no issues
  3. After we merge #82 we rebase the semi standard adoption over that, changing only the lines of code you add in #82

Thoughts?

baffo32 commented 8 years ago

I think it would definitely be helpful to separate the patch from the restyling.

If your restyling is simply a run of semistandard --fix, then I can run that on my branch, too, and it will become similar and the merge may become relatively painless. But if the restyling is mostly manual changes that aren't easily reproducible, then one of us will have to go through and manually propagate things by hand.

andrewjaykeller commented 8 years ago

Yea there was a bunch of manual changes. Let's go with my three step suggestion and you can essentially ignore this for the time being!

baffo32 commented 8 years ago

Could you merge this into either master or a new 1.4.0 branch, so that changes for 1.4.0 can be separated out into different pull requests and commits?

I think for tests to run smoothly for #82 some other fixes will be needed first. Those fixes should probably happen on top of this branch, since they would be new code.

andrewjaykeller commented 8 years ago

@baffo32 done!