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

Feature/add preid param for semver #83

Conversation

stvnwrgs
Copy link

@stvnwrgs stvnwrgs commented Aug 8, 2016

New argument -p allows you to create prereleases. You can also define a prerelease identifier. via --pre-id="alpha" (beta is default).

default: false,
global: true
})
.option('pre-id', {
Copy link
Member

@stevemao stevemao Aug 8, 2016

Choose a reason for hiding this comment

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

I think this should be called "tag", as https://docs.npmjs.com/cli/publish

Also you need to change the final npm publish information

Copy link
Author

@stvnwrgs stvnwrgs Aug 8, 2016

Choose a reason for hiding this comment

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

Defining a tag when publishing is the same like a git tag. pre-release and pre-id are one part of the tag. See http://semver.org/ for more infos.

About the publish, yes but this is in the users scope. If you want to use alpha, beta instead of dist-tags f.e, it should be configurable and the user can choose how and where he publishes what. That's one of the great things of standard-version, you can also use if for a non node project.

Copy link
Member

Choose a reason for hiding this comment

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

Where did you see the name "pre-id"?

About the publish, yes but this is in the users scope.

What I mean is just to display a recommendation. Because right now it says npm publish without a tag. Displaying --tag would be a better recommendation.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not completely sure about the relationship between git tag and npm publish tag. It doesn't look the same to me. npm publish by default uses "latest" tag.

Copy link
Author

@stvnwrgs stvnwrgs Aug 10, 2016

Choose a reason for hiding this comment

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

Sorry, @stevemao you are right. The npm publish tag is not the same like the git tag. I will also rename it to tag.

@stevemao
Copy link
Member

stevemao commented Aug 8, 2016

I wonder if we could leverage unreleased option in conventional-changelog.

@@ -1,2 +1,3 @@
node_modules
.nyc_output
.idea
Copy link
Member

Choose a reason for hiding this comment

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

Don't do unrelated changes :)

@stvnwrgs
Copy link
Author

stvnwrgs commented Aug 10, 2016

After using it for a while, I found two bugs that need's to be fixed before this can be merged.

One is nearly fixed, the second one needs some discussion.

Should prereleases be shown in the changelog?
I think changelogs should only be written for no prereleases.

sl-bot added 2 commits August 10, 2016 19:10
feat(args): --next (-x) --actual (-a) now print out the version without doing something
@stvnwrgs stvnwrgs mentioned this pull request Aug 10, 2016
@stevemao
Copy link
Member

What bugs? Can you remove all unrelated changes and separate different things into different issues?

@stvnwrgs
Copy link
Author

stvnwrgs commented Aug 11, 2016

The bugs should be fixed with the two latest commits. If you tried to create a prerelease and added a new feature, it did not increase the minor version instead, it just increased the prerelease, which was wrong.

The only unrelated thing right now is the next, actual feature. I could remove it, wait until the PR is merged and read it, but its just 15 lines of code. In my opinion, it's not worth the time. But if you insist, I will do it.

Should I remove the changelog entry and downgrade the version to 2.4.0 again?

@stevemao
Copy link
Member

I'd suggest you to create another PR for other things. You also need to add tests. Also too many unnecessary whitespace changes.

@coveralls
Copy link

coveralls commented Aug 11, 2016

Coverage Status

Coverage decreased (-0.9%) to 94.0% when pulling df702f7 on StyleLounge:feature/add-preid-param-for-semver into 0b28a25 on conventional-changelog:master.

@stvnwrgs
Copy link
Author

@stevemao: What about the package.json and CHANGELOG.md? I think it does not make sense to generate them in the PR, right?

@coveralls
Copy link

coveralls commented Aug 11, 2016

Coverage Status

Coverage decreased (-1.3%) to 93.617% when pulling c609595 on StyleLounge:feature/add-preid-param-for-semver into 0b28a25 on conventional-changelog:master.

@stevemao
Copy link
Member

Yeah, as atomic as possible :)

@stvnwrgs
Copy link
Author

Should be ready now. Thanks for your help!

@coveralls
Copy link

coveralls commented Aug 11, 2016

Coverage Status

Coverage decreased (-1.3%) to 93.617% when pulling 3ad4962 on StyleLounge:feature/add-preid-param-for-semver into 0b28a25 on conventional-changelog:master.

checkpoint('bumping version in package.json from %s to %s', [pkg.version, newVersion])
pkg.version = newVersion
fs.writeFileSync(pkgPath, JSON.stringify(pkg, null, 2) + '\n', 'utf-8')
} else {
checkpoint('skip version bump on first release', [], chalk.red(figures.cross))
}

outputChangelog(argv, function () {
// only commit the package.json on prerelease
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Author

Choose a reason for hiding this comment

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

So the version for a prerelease is bumped, but no docs are generated. This only happens when the release is stable. It would flood the changelog otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@stvnwrgs, I think I'm with @stevemao. Let's generate a CHANGELOG for pre-releases, with this caveat:

It would be neat if there was eventually logic that could once v1.0.0 is released, combine the information from v1.0.2.alpha.0, v1.0.3.alpha.1 into a single v1.0.0 entry in the CHANGELOG; I would start simple though and not have a special case for pre-release handling.

Copy link
Member

Choose a reason for hiding this comment

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

I'm with @bcoe, the end goal should be combining them to a single entry eventually. Another option is to collect the changes in a Unrelease vX.Y.Z and then name it to vX.Y.Z once it's not a pre-release anymore. I could swear I've seen that around, although I'm too lazy now to search for it.

Copy link
Member

@Tapppi Tapppi Oct 5, 2016

Choose a reason for hiding this comment

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

Now that I'm rambling on I realise that this is a result of the workflow, not this change, never mind me.. Anyway for reference:

@bcoe I thought about combining the pre-release logs to a single one, but what about pre-releases containing fixes to other pre-releases. e.g. You do a pre-release of a new feature as v2.1.0-rc.0 with feat(god): can now obtain god powers, but then fix an edge case of this new feature in the next prerelease v2.1.0-rc.1: fix(god): fix edge case to give full god instead of demigod powers. Both of them end up in the combined v2.1.0 changelog and it might end up with quite a lot of changes that are out of context.

standard-version -p --tag="<identifier>"
```

### Dry run
Copy link
Member

Choose a reason for hiding this comment

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

Remove this for this PR?

Copy link
Author

Choose a reason for hiding this comment

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

Why should documentation for new features of a PR not be included?

Copy link
Member

Choose a reason for hiding this comment

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

The code for this is not in this PR yet

@stevemao
Copy link
Member

I wonder if we should let conventional-recommended-bump to decide if this is a prerelease?

@stevemao
Copy link
Member

stevemao commented Aug 12, 2016

There is also a from-git option in npm-version

@stvnwrgs
Copy link
Author

Really, after 4 days you come up conventional-recommended-bump ?

@stevemao
Copy link
Member

What do you mean? I'm just saying if conventional-recommended-bump can detect prereleases

}
} else {
newVersion = semver.inc(newVersion, 'prerelease', argv.tag)
console.log('normal: ' + newVersion)
Copy link
Member

Choose a reason for hiding this comment

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

let's drop this log line.

@bcoe
Copy link
Member

bcoe commented Aug 23, 2016

@stvnwrgs this is awesome work, would love to work with you to get it merged; I've been balancing standard-version with a few other OSS projects, and apologize that the turn around time on our issues can sometimes be moderately frustrating.

@stevemao stevemao mentioned this pull request Sep 15, 2016
@Tapppi
Copy link
Member

Tapppi commented Sep 17, 2016

This feature would be awesome. @stvnwrgs do you have interest in continuing work on this, or do you prefer someone else continue if they have time? I intend to review this more thoroughly as soon as I have a slot of time open.

@AntJanus
Copy link

AntJanus commented Oct 4, 2016

@Tapppi @stvnwrgs I'd be really interested in seeing this feature pass. What else is left? Maybe I can fork + contribute

@stevemao
Copy link
Member

stevemao commented Oct 4, 2016

@AntJanus you are more than welcome to pick up the work 😄 open source projects are about community 😃

@stvnwrgs
Copy link
Author

stvnwrgs commented Oct 5, 2016

@AntJanus I'm out. Feel free!

@bcoe
Copy link
Member

bcoe commented Oct 6, 2016

@AntJanus I'd love to see this feature too, perhaps you can start by opening an issue; and we can discus what we'd like to see out of pre-release functionality? Then we can help see @stvnwrgs hard work over the finish line.

@AntJanus
Copy link

AntJanus commented Oct 6, 2016

@bcoe Opened an issue on #122

@bcoe
Copy link
Member

bcoe commented Nov 26, 2016

closing in favor of #129 which looks almost ready to land.

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.

6 participants