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

feat: added support for commitAll option in CLI #121

Merged
merged 5 commits into from
Oct 6, 2016

Conversation

benmonro
Copy link
Contributor

@benmonro benmonro commented Oct 6, 2016

npm version will commit any staged files as part of it's execution. This feature adds the option to support that in build server scenarios where a build step needs to commit other files besides just package & changelog. the default behavior is preserved, so this is an opt in feature.

@coveralls
Copy link

coveralls commented Oct 6, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling c9c10de on benmonro:patch-1 into ce268d9 on conventional-changelog:master.

Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

love this idea, and the tests are great.

@bcoe
Copy link
Member

bcoe commented Oct 6, 2016

@Tapppi @stevemao what do you think; I like this feature, and think the tests and updates are ready to land.

handledExec(argv, 'git add package.json ' + argv.infile, cb, function () {
handledExec(argv, 'git commit ' + verify + (argv.sign ? '-S ' : '') + 'package.json ' + argv.infile + ' -m "' + formatCommitMessage(argv.message, newVersion) + '"', cb, function () {
handledExec(argv, 'git commit ' + verify + (argv.sign ? '-S ' : '') + (argv.commitAll ? '' : ('package.json ' + argv.infile)) + ' -m "' + formatCommitMessage(argv.message, newVersion) + '"', cb, function () {
Copy link
Member

Choose a reason for hiding this comment

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

This line is a bit messy. I'd like to use a variable. But doesn't have to be in this PR :)

Copy link
Member

Choose a reason for hiding this comment

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

We can refactor this in a separate PR.

@@ -101,9 +102,8 @@ function commit (argv, newVersion, cb) {
args.unshift('package.json')
}
checkpoint(argv, msg, args)

Copy link
Member

Choose a reason for hiding this comment

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

This change seems a bit unnecessary to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it wasn't intentional. I'll remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I readded the whitespace, not sure why but it's not showing up in the diff here.

@stevemao
Copy link
Member

stevemao commented Oct 6, 2016

Generally LGTM :)
Thanks a lot!

Copy link
Member

@Tapppi Tapppi left a comment

Choose a reason for hiding this comment

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

The empty line change seems a bit weird, but otherwise LGTM! Thanks for your contribution.

handledExec(argv, 'git add package.json ' + argv.infile, cb, function () {
handledExec(argv, 'git commit ' + verify + (argv.sign ? '-S ' : '') + 'package.json ' + argv.infile + ' -m "' + formatCommitMessage(argv.message, newVersion) + '"', cb, function () {
handledExec(argv, 'git commit ' + verify + (argv.sign ? '-S ' : '') + (argv.commitAll ? '' : ('package.json ' + argv.infile)) + ' -m "' + formatCommitMessage(argv.message, newVersion) + '"', cb, function () {
Copy link
Member

Choose a reason for hiding this comment

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

We can refactor this in a separate PR.

@@ -92,6 +92,29 @@ describe('cli', function () {
content.should.match(/1\.0\.1/)
content.should.not.match(/legacy header format/)
})

it('commits all staged files', function () {
Copy link
Member

Choose a reason for hiding this comment

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

Love the test, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks :)

@coveralls
Copy link

coveralls commented Oct 6, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 0a0eca7 on benmonro:patch-1 into ce268d9 on conventional-changelog:master.

@benmonro
Copy link
Contributor Author

benmonro commented Oct 6, 2016

can we merge this guys? our builds are doing 2 commits per build and it'd be really nice to updated to this ASAP :)

@bcoe bcoe merged commit a903f4d into conventional-changelog:master Oct 6, 2016
@benmonro
Copy link
Contributor Author

benmonro commented Oct 6, 2016

Thanks @bcoe does this get automatically published to npm?

@bcoe
Copy link
Member

bcoe commented Oct 6, 2016

@benmonro this is the dream eventually:

https://github.com/bcoe/travis-deploy-example

But currently there's some meat in the machine, give me 5 minutes.

@benmonro
Copy link
Contributor Author

benmonro commented Oct 6, 2016

👍 Thanks!

@bcoe
Copy link
Member

bcoe commented Oct 6, 2016

@benmonro give this a spin:

+ standard-version@3.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants