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

Add cross-platform support for pre/post version script hooks. #17

Merged

Conversation

webuniverseio
Copy link
Contributor

Also fix error reporting for exec function that was failing when result stdout and stderr are null (for example in case when ENOENT error is surfaced). Fixes: #2

Fix error reporting for exec function that was failing when result `stdout` and `stderr` are null (for example in case when `ENOENT` error is surfaced).
Fix error reporting for bump prompt cli.
@webuniverseio
Copy link
Contributor Author

@BigstickCarpet , what do you think?

@JamesMessinger
Copy link
Member

@szarouski - Thanks for the PR. It looks good, but if you could add a couple more things, I'd really appreciate it.

Script order
To fully reproduce the behavior of npm version, it should perform the steps that are listed here. Specifically:

  1. Run the preversion script before the version is updated (and before the version prompt is shown)
  2. Run the version script after the version is updated, but before git commit and git tag
  3. Run the postversion script after git commit and git tag, but before git push

Tests
Please add a few tests for this new functionality. The tests currently work by modifying the PATH variable to run a dummy git binary that just writes its arguments to a text file. Then this function is called to assert that git was run with the expected arguments. The same approach should be taken with npm, so we can assert that npm is being called as expected.

@webuniverseio webuniverseio force-pushed the feature/pre-post-version branch 8 times, most recently from 7719b2f to 67e0340 Compare December 4, 2016 21:08
@webuniverseio
Copy link
Contributor Author

@BigstickCarpet Ok, I'll try. First I had to make tests run on windows: https://ci.appveyor.com/project/szarouski/version-bump-prompt. I added appveyor CI (with a little bit of pain). Would you mind dropping support for node < 4? I can make it working, but it require more time and I'm not sure it is much needed. Worst case people can submit another PR.

@webuniverseio
Copy link
Contributor Author

@BigstickCarpet Please review again.

@JamesMessinger
Copy link
Member

JamesMessinger commented Dec 8, 2016

@szarouski - Thanks for all your work on this. I haven't had a chance to review your latest commits yet (been too busy at work), but should be able to look at them next week. Sorry for the delay :(

Thanks for adding appveyor CI. I've been meaning to do that anyway. 👍

I'm fine with dropping support for old Node versions (< 4.0). Just update the engines settings in the package.json file accordingly.

Thanks again!

@webuniverseio
Copy link
Contributor Author

No problem, I'm happy to contribute 😄. I updated engines setting.

@JamesMessinger
Copy link
Member

@szarouski - I've finally got some free time, so I'm merging this PR now. Will make some additional changes and then publish it to npm as v3.0.0

@JamesMessinger JamesMessinger merged commit 69347a3 into JS-DevTools:master Dec 24, 2016
@JamesMessinger
Copy link
Member

@szarouski - v3.0.0 has been published to npm. Thanks again for your help! I gave you credit in the changelog 👍

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.

Support npm script preversion, version, and postversion
2 participants