Cool idea - Githubissues

bchr02 / node-pre-gyp-github

A node-pre-gyp module which provides the ability to publish to GitHub releases.

MIT License

52 stars 32 forks source link

Cool idea #3

Closed cscott closed 6 years ago

cscott commented 9 years ago

I've been doing something similar for a while; see: https://github.com/cscott/node-icu-bidi https://github.com/cscott/node-php-embed etc.

Perhaps you can borrow some ideas from my scripts? I've been too lazy to actually package everything up as a nice standalone npm packages, glad to see you're doing that.

bchr02 commented 9 years ago

Thanks @cscott. I'll take a look.

springmeyer commented 8 years ago

I like @cscott's use of node-pre-gyp reveal which should ensure that the right local paths are used no matter the variety of options you might use (Debug builds, various archs, etc). In a quick look around node-pre-gyp-github I don't see

tests
continuous integration
or the use of reveal

So I'm hesitant to recommend this tool yet. I landed here trying to answer the question of whether it makes sense to recommend using this tool via the node-pre-gyp README (pre https://github.com/mapbox/node-pre-gyp/pull/108#issuecomment-151478779).

bchr02 commented 8 years ago

@springmeyer Thanks for getting back to me on this. It makes sense to add each of those items, so I will do so.

Task List:

[x] tests (added with https://github.com/bchr02/node-pre-gyp-github/commit/73e8be34cd3d5fcb910b5c925a6b25af81dddb14)
[ ] continuous integration
[ ] use of reveal

springmeyer commented 8 years ago

great @bchr02. The simplicity of https://github.com/cscott/node-icu-bidi/blob/master/scripts/publish.js is also appealing so you might look for other smart things it does?

bchr02 commented 8 years ago

The simplicity of https://github.com/cscott/node-icu-bidi/blob/master/scripts/publish.js is also appealing so you might look for other smart things it does?

okay, I will do that.

bchr02 commented 8 years ago

@springmeyer with https://github.com/bchr02/node-pre-gyp-github/commit/73e8be34cd3d5fcb910b5c925a6b25af81dddb14 I have just added unit tests with over 97% line coverage. I still plan on adding CI and reveal, but maybe now this is enough for you to recommend this module. :smile:

bchr02 commented 8 years ago

@springmeyer now at 100% coverage!

springmeyer commented 8 years ago

Thanks @bchr02 - Yes, it is. I'm traveling - perhaps you could make a PR against node-pre-gyp adding mention and usage to the readme?

bchr02 commented 8 years ago

@springmeyer Sorry for taking so long. As you requested, I have made mention and submitted https://github.com/mapbox/node-pre-gyp/pull/216 Thank you.