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

support releasing a custom version, including pre-releases #129

Merged
merged 7 commits into from
Nov 26, 2016

Conversation

e-cloud
Copy link
Contributor

@e-cloud e-cloud commented Oct 15, 2016

major changes

  • add a option --release-as to release a custom version like npm version
  • add a option --prerelease to name a pre-release or tag a release as prerelease without option value, it can be used with --release-as

this PR is similar to #83 , but this provide more version-control.


PS: some of the test code are borrowed from tests for #83

PPS: I'm bad at naming the option. Feel free to suggest new names for those options.

@coveralls
Copy link

coveralls commented Oct 15, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling f793bc5 on e-cloud:master into 8e4ec4a on conventional-changelog:master.

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.

So this is related to #83 (Preid PR) #84 (releaseAs issue) and #122 (Preid Issue).

Thanks for contributing. A few concerns I have:

  • tag-id should IMO be the --prerelease [-p] that was discussed in Feature/add preid param for semver #122. This option should convert the bump to a pre(major|minor|patch) as in Feature/add preid param for semver #83. Otherwise you are forced to manually find out what version you want to prerelease and use it with the manual|release-as option, kind of going against the idea of standard-version. To set the prerelease tag you would -p alpha.
  • manual|release-as would only need to accept major|minor|patch as input, since the -p flag should set it as a prerelease when no manual|release-as is given and otherwise -p would set it as premajor|preminor|prepatch .

What this would entail is:

  • Either getting the recommended bump from conventional-recommended-bump or using the (major|minor|patch) from (manual|release-as) flag.
  • Prefixing the bump with pre if prerelase flag is used, and supplying either the given or default prerelease tag to semver.inc.

If the above doesn't resolve prereleases with breaking changes (e.g. patch prerelease into (major|minor) prerelease) correctly, it'll need a modified version of this function in #83.

/CC @bcoe @stevemao @nexdrew

@@ -3,7 +3,25 @@
root = true

[*]
Copy link
Member

Choose a reason for hiding this comment

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

Could you create a separate PR for modifying the .editorconfig? Thanks 😄

@@ -4,6 +4,21 @@ var defaults = require('./defaults')

var argv = require('yargs')
.usage('Usage: $0 [options]')
.option('manual', {
Copy link
Member

Choose a reason for hiding this comment

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

release-as [-r] would be a good name IMO (@stevemao opinion?).

choices: ['major', 'premajor', 'minor', 'preminor', 'patch', 'prepatch', 'prerelease'],
global: true
})
.option('tag-id', {
Copy link
Member

Choose a reason for hiding this comment

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

prerelease would be my preference, see the summary comment on why.

version[type] += 1

var content = fs.readFileSync('CHANGELOG.md', 'utf-8')
content.should.match(new RegExp(version.major + '.' + version.minor + '.' + version.patch))
Copy link
Member

Choose a reason for hiding this comment

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

The dots in the regex should be escaped. E.g. version.major + '\.' + version.minor + '\.' + version.patch.

version[type.slice(3)] += 1

var content = fs.readFileSync('CHANGELOG.md', 'utf-8')
content.should.match(new RegExp(version.major + '.' + version.minor + '.' + version.patch + '-' + type + '.0'))
Copy link
Member

Choose a reason for hiding this comment

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

The dots in the regex should be escaped. E.g. version.major + '\.' + version.minor + '\.' + version.patch.

})
})

it('creates a prerelease with a new minor version after two prerelease patches', 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 works correctly when specifying the preminor manually, but how about prerelease? Would that bump the patch version? That would be very counterintuitive. This might not be worth fixing before deciding on the points in my summary though.

@e-cloud
Copy link
Contributor Author

e-cloud commented Oct 17, 2016

@Tapppi all your comment can be worked out except this:

Prefixing the bump with pre if prerelase flag is used, and supplying either the given or default prerelease tag to semver.inc.

That involves some problems:

  • yargs can specify a default value for an option, but can't not set as undefined when not using that option in cli. Then we aren't able to see if user specify a prerelease flag with simple way
  • when someone wants no tag id, with default tag-id, it would also be hard to implement the requirement

so, the default value for prerelease is undefined and when a use specify it with option value, its value would be ''(empty string). If a user need a custom tag id, he/she should specify it explicitly for now.

@coveralls
Copy link

coveralls commented Oct 17, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling e3a1892 on e-cloud:master into 8e4ec4a on conventional-changelog:master.

@@ -2,12 +2,12 @@
"name": "standard-version",
"version": "3.0.0",
"description": "replacement for `npm version` with automatic CHANGELOG generation",
"bin": "cli.js",
"bin": "bin/standard-version.js",
Copy link
Member

Choose a reason for hiding this comment

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

the naming needs to be changed. cli should be the bin. So you'd need to rename the files to whatever you think makes more sense :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but why?

Copy link
Member

Choose a reason for hiding this comment

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

cli by convention should be the bin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it. what if i rename bin/standard-version.js to bin/cli.js and cli.js to command.js?

@e-cloud e-cloud force-pushed the master branch 2 times, most recently from 06d413c to a111a4b Compare October 26, 2016 02:25
@coveralls
Copy link

coveralls commented Oct 26, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling a111a4b on e-cloud:master into 8e4ec4a on conventional-changelog:master.

@Tapppi Tapppi mentioned this pull request Oct 26, 2016
@e-cloud
Copy link
Contributor Author

e-cloud commented Nov 8, 2016

@stevemao do you have time to review this again?

describe('release-types', function () {
var regularTypes = ['major', 'minor', 'patch']

regularTypes.forEach(function (type) {
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't auto generate tests like this. I'd keep it DAMP.

Copy link
Contributor Author

@e-cloud e-cloud Nov 22, 2016

Choose a reason for hiding this comment

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

But in this case we dont add a bunch of fixtures to avoid duplication. I think its readability is acceptable, just opinion. If you insist, i can expand it. I would be much longer.

Copy link
Member

Choose a reason for hiding this comment

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

I'd probably just test one for each assertion. EG: say test 'major' for "creates a major release" and 'patch' for "creates a pre-patch release". Not a big deal :)

@stevemao
Copy link
Member

stevemao commented Nov 13, 2016

it looks awesome In general. Thanks, @e-cloud.
We also need to add docs before we can merge :)
I'd also like to hear other maintainers opinions :)

this is for better modularity, and, for more convenient test.

before, you can't breakpoint-debug the source mostly when testing for
the usage of shelljs.
now, some of the test cases can be done potentially with this commit
Here uses promise for some of the test cases, works nicely, thanks bluebird.
@coveralls
Copy link

coveralls commented Nov 22, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling d784d46 on e-cloud:master into 80004ec on conventional-changelog:master.

@e-cloud
Copy link
Contributor Author

e-cloud commented Nov 25, 2016

I just add docs in Readme.md. Anybody could review this?

@bcoe
Copy link
Member

bcoe commented Nov 26, 2016

@e-cloud really appreciate this work; sorry for the slow turn around on this repo lately, going to work on seeing this over the finish line 👍

@bcoe bcoe closed this Nov 26, 2016
@bcoe bcoe reopened this Nov 26, 2016
@coveralls
Copy link

coveralls commented Nov 26, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling d784d46 on e-cloud:master into 80004ec on conventional-changelog:master.

@bcoe
Copy link
Member

bcoe commented Nov 26, 2016

@stevemao @Tapppi sorry that I've been so AWOL the past couple months -- haven't been able to carve as much OSS time out as I would like to.

Please feel free to land any pull requests that meet your guidelines; I've added you both to the npm module so that I don't block you on publishing, and I very much trust your instincts with regards to features.

@bcoe bcoe merged commit 068008d into conventional-changelog:master Nov 26, 2016
@bcoe bcoe mentioned this pull request Nov 26, 2016
@bcoe
Copy link
Member

bcoe commented Nov 26, 2016

@e-cloud @Tapppi give this a spin when you have a moment:

npm cache clear; npm i standard-version@next

I've landed these changes in a pre-release! how meta is that.

@Tapppi
Copy link
Member

Tapppi commented Dec 1, 2016

@bcoe @e-cloud sorry to bump this, but this is fricking awesome, I just released my first pre-release of an internal company package automagically from our CI server after versioning with standard-version@4.0.0-0 🎉 😍 ! All the changes look pretty good, I don't think there are any blocking issues for v4?! :shipit:

@bcoe
Copy link
Member

bcoe commented Dec 2, 2016

@Tapppi if you're comfortable signing off on everything, I say release it! my two cents:

  • I think your point about rolling back the operation if an error is a good thing for us to aspire to; but perhaps we can take this on as we start to abstract the checkpoint logic... maybe model after database migrations? have a rollback for each checkpoint.
  • I love the pre-release functionality, my only beef is changelog management gets slightly weird if you release several pre-releases in a row -- but I think this is something we can work on in the future too.

want to cut a release @Tapppi you should have access to the module on npm now too.

@Tapppi
Copy link
Member

Tapppi commented Dec 2, 2016

@bcoe I think both of those points are crucial but big enough to warrant thinking a bit more about them instead of trying to rush them in. I'll give releasing v4.0.0 a shot right now, thanks! 👍

@Tapppi Tapppi mentioned this pull request Dec 23, 2016
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