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

Merge this small changes with the "/sandbox/rzr/abandonware/review/master" in order to release it as @abadonware/bleno on npm #1

Merged

Conversation

petersaints
Copy link

I have just tested the changes @rzr has made to bleno on the "sandbox/rzr/abandonware/review/master" branch. Everything worked fine for me under Node 10.15.0. I just made a couple of changes:

  • I renamed the package from "bleno" to "@abandonware/bleno" so that it can be published without clashing with the original package and versioned it as 0.5.1-0 (I think that the scheme you're using)
  • I also updated the dependencies to their latest versions.

As far as I'm concerned, I think you can publish it on npm. If anything else arises I'll push more changes.

@rzr
Copy link

rzr commented Mar 5, 2019

May I suggest you to rebase on my sandbox branch add your changes and then rebase on master , then I can merge it.

Then feel free to apply for:
abandonware/noble#9

@petersaints
Copy link
Author

I think that I have already did (mostly) what you asked. I'm not a Git expert.

Either way, please note that if you intend to publish it to npm you'll have to remove the ""private": true" option that I added to my fork.

I did that because I use npm update would give me an error when I try to update my projects that link to my local copy of bleno. It would try to find @abadonware/bleno on NPM, but it would obviously fail to find it.

@rzr
Copy link

rzr commented Mar 5, 2019

Rebase is a great feature , If you're not familiar I can assist you to.

I can also make your task easier by picking the changes from your branch too.

Give a couple of hours and I do that

@rzr
Copy link

rzr commented Mar 5, 2019

I updated master with some of your changes, feel free submit your serie over it (without merge),
BTW Is npm link better than renaming it to abandonware/bleno and publish it to npm ?

@petersaints
Copy link
Author

Ok. But when you say submit my series over it, you mean a new pull request, right? Because I don't think I can change the branch I'm basing this pull request on ("/sandbox/rzr/abandonware/review/master") to master.

Regarding npm link vs publishing, I'm just using it as a local replacement for the package. I still think it may be useful to have the package available on npm. With npm link people would have to clone the repository to their on PCs in order to link to it. With a proper npm package they can just run npm install @abandonware/bleno.

@rzr
Copy link

rzr commented Mar 5, 2019

Ok. But when you say submit my series over it, you mean a new pull request, right? Because I don't think I can change the branch I'm basing this pull request on ("/sandbox/rzr/abandonware/review/master") to master.
you can use

git remote add upstream https://github.com/abandonware/bleno
git fetch --all
git rebase -i remotes/upstream/master
then
git push --force origin

this PR will be updated automagicaly

Regarding npm link vs publishing, I'm just using it as a local replacement for the package. I still think it may be useful to have the package available on npm. With npm link people would have to clone the repository to their on PCs in order to link to it. With a proper npm package they can just run npm install @abandonware/bleno.

previous packages were uploaded to @abandonware org

see https://www.npmjs.com/~rzr

I think I will follow the same pattern, so you can drop the private:true flag

@petersaints
Copy link
Author

Ok! I think I had already done what you asked using GitKraken (sometimes I use the GUI, sometimes CLI). But I went through all the steps again and it seems ok.

Yes, I think that it is a nice idea to have all noble related packages working with the Node 10 LTS under the @abandonware org.

I can't promise that I'm able to maintain and improve upon noble, bleno, node-bluetooth-hci-socket, etc., but I usually move to the latest Node LTS when it comes out and if something breaks again I will try to fix it and push my changes.

package.json Outdated Show resolved Hide resolved
package.json Outdated
@@ -35,13 +35,13 @@
"license": "MIT",
"readmeFilename": "README.md",
"devDependencies": {
"jshint": "~2.9.4",
"jshint": "~2.9.7",
Copy link

Choose a reason for hiding this comment

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

maybe do this in separate change if upstream want to pick this

Copy link
Author

Choose a reason for hiding this comment

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

Sure. I'm reverting those changes.

However, do you want to keep the jshint fixes? Basically I added some missing commas and renamed two variables that were still in scope due to JavaScript's variable hoisting. Since jshint is being run as a pretest step "npm test" was failing. That triggered my OCD and I had to fix it! If you think those changes is something upstream may be interested in adopting I'll keep them, otherwise we can drop them.

Copy link

@rzr rzr left a comment

Choose a reason for hiding this comment

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

check versions

@petersaints
Copy link
Author

check versions

It should be Ok now. Just check this comment before proceeding: #1 (comment)

@rzr
Copy link

rzr commented Apr 15, 2019

Any other change to suggest for next release ?

I am considering to pick:
noble#410

Relate-to: https://abandonware.github.io/

@petersaints
Copy link
Author

petersaints commented Apr 15, 2019 via email

@rzr rzr merged commit cb7c258 into abandonware:sandbox/rzr/abandonware/review/master Jul 23, 2019
@rzr
Copy link

rzr commented Jul 23, 2019

Sorry I made similar changes on master branch and released to:
https://www.npmjs.com/package/@abandonware/bleno/v/0.5.1-0

Please rebase and submit patches if needed

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