-
Notifications
You must be signed in to change notification settings - Fork 122
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
Fixes issue 42 and 94. #125
Conversation
Bumping to the next increment should bump a prerelease to a full release so |
@@ -1,7 +1,7 @@ | |||
{ | |||
"name": "grunt-bump", | |||
"description": "Bump package version", | |||
"version": "0.1.0", | |||
"version": "0.1.1-rc0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is done automatically
@eddiemonge, re the need for bump:release, True, but it seems strange to me that from 1.0.0-rc.0 bump:major, bump:minor, and bump:patch all result in 1.0.0. I'll remove it if you like, but it seemed to me to be confusing, and not necessarily what someone would think they were going to do by bumping a version. |
The prerelease should be for the release that is before the dash, so:
Going from |
@eddiemonge I think I've addressed all of the issues you brought up and fixed the REGEXP problem. Is there anything else you'd like done? |
Commits should be squashed down to one and use the contributing guidelines for the title and message. Can you also make sure that you have incorporated everything from https://github.com/vojtajina/grunt-bump/pull/102/files ? thanks! |
Should be all set. I added to the readme for the changes, not sure if you wanted that in a separate commit let me know if you want it separated out? |
Im not sure |
>> Tagged as "v1.0.2-rc.0" | ||
>> Pushed to origin | ||
|
||
$ grunt bump:prerelease |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these examples are a little confusing. why go from prepatch to prerelease?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: examples/ expected behavior of prerelease
bump:prerelease is how you bump the prerelease version.
From a released version (1.0.0) you choose
- prepatch -> 1.0.1-rc.0 prerelease -> 1.0.1-rc.1
- preminor -> 1.1.0-rc.0 prerelease -> 1.1.0-rc.1
- premajor -> 2.0.0-rc.0 prerelease -> 2.0.0-rc.1
The way it is now (and in the readme) is incorrect.
- Current behavior. v1.0.0 bump:prerelease -> v1.0.0-1
1.0.0-1 should be the prerelease for 1.0.0 so this should not be the expected behavior.
re: the defaulted prreleaeName.
There needs to be something other than an empty string as the prereleaseName otherwise the the versions are not bumped properly. They behave more like he existing functionality of prerelease which is incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From Semver 2.0.0:
A pre-release ... Identifiers MUST NOT be empty. ... Examples: 1.0.0-alpha, 1.0.0-alpha.1, 1.0.0-0.3.7, 1.0.0-x.7.z.92.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed:
A pre-release version MAY be denoted by appending a hyphen and a series of dot separated identifiers immediately following the patch version
Meaning everything after the hyphen is the identifier. To have multiple identifiers, you separate them with dots.
As for the current current behavior of going from 1.0.0
to 1.0.0-1
as the next prerelease, that is a bug that existed in semver which has since been fixed. Purely upgrading semver with no other changes would make it go from 1.0.0
to 1.0.1-0
.
I see: Now defaulted to |
I'd expect that last one to go: 1.0.1-0 bump:prerelease to 1.0.1-1 But maybe that would be bump:prepatch. Otherwise, how do you increment a prerelease? |
03523f3
to
e622494
Compare
Agreed. After making some more changes to the REGEXP, it is now working as expected for prereleases both with and without a prerelease identifier.
with identifier:
If you had previously used bump:git and are now at a version like 1.0.0-12-g10rwy where 12 is the number of commits from the tag 1.0.0
|
Hmm that last git part is interesting. I wonder if there is a way to fix that, or if its something that can be ignored but with a note in the readme. edit nevermind I see you already added the note. However, the readme needs to be updated. The default prereleaseName is null so not passing one in shouldn't add the I'm also a bit hesitant about changing the regex. I'd actually prefer there not to be a regex. I want to say semver will the do that for us but that might be an additional change that needs to happen independent of this commit |
Not sure what you mean by ignored? I would suggest advising against bumping from a git version to a prerelease version (without a prerelaseName?) (What would you expect when bumping from a 'git version' to a prerelease version?) Thoughts? Update
Re: the RexExp would issue 119 be part of the solution to this. I'd agree that removing the regexp all together would be part of another commit/issue. (though while the functionality of bump:git is still an option I'm not sure the RegExp could be removed, probably could be simplified if issue #119 was addressed?) |
Upgrade semve to allow for prerelease versions Change RexExp for versions and prerelease versions Add prereleaseName to options, defaulted to null - pre release types: named: 1.0.0-rc.0 unnamed: 1.0.0-0 Fix issue where bump:git on multiple files failed Closes: 42, 94, 102, 110
Are there any further changes you would like to see before you're ready to accept this Pull Request? |
@@ -77,14 +82,12 @@ module.exports = function(grunt) { | |||
|
|||
var globalVersion; // when bumping multiple files | |||
var gitVersion; // when bumping using `git describe` | |||
var VERSION_REGEXP = /([\'|\"]?version[\'|\"]?[ ]*:[ ]*[\'|\"]?)([\d||A-a|.|-]*)([\'|\"]?)/i; | |||
|
|||
var VERSION_REGEXP = new RegExp('([\\\'|\\\"]?version[\\\'|\\\"]?[ ]*:[ ]*[\\\'|\\\"]?)(\\\d+\\\.\\\d+\\\.\\\d+(-'+opts.prereleaseName+'\\\.\\\d+)?(-\\\d+)?)[\\\d||A-a|.|-]*([\\\'|\\\"]?)', 'i'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
holy smokes batman, that is one scary regex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you get rid of all the escapes (that are there so we can add a variable) it's much less scary looking. I tried to write a couple of tests, but am unfamiliar with testing node modules.
I could just copy the regex over to a test for now (though not a great strategy). If you have any suggestions on how to add tests for the regex I'd be happy to flush them out.
I have tested this quite a bit manually. Let me know if there's anything I can do to help out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didnt notice this before, but shouldn't the regex replace:
(-'+opts.prereleaseName+'\\\.\\\d+)?
with something that won't translate to: (-null\\\.\\\d+)?
like:
(-'+ (opts.prereleaseName || '') +'\\\.\\\d+)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schechter ^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so.
When there is a prereleaseName the format is 1.0.0-.0
Without a name the format is different, there is not just, just the dash so 1.0.0-0 so the name || '' would not match.
I am scared of this pr. Let me do a bit of testing to make sure it works as expected. |
// BUMP ALL FILES | ||
runIf(opts.bumpVersion, function() { | ||
grunt.file.expand(opts.files).forEach(function(file, idx) { | ||
var version = null; | ||
var content = grunt.file.read(file).replace(VERSION_REGEXP, function(match, prefix, parsedVersion, suffix) { | ||
gitVersion = gitVersion && parsedVersion + '-' + gitVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part got left off so bump:git isn't working. #129 Working on fixing it.
How is it not working? bump:git is working for me. I believe that part is what broke bump:git on multiple files. What is is doing that is unexpected? bump:git will set the version to the result of git describe if it was parsedVersion i.e. |
The I fixed it in a2e06c1 |
Yes, that's correct. bump:git is now broken. a2e06c1 broke bump:git again. It is not behaving as it did prior to it bring broken by 3679aecf8c As the comment on that commit states,
bump:git is now bumping the patch version and not behaving as documented in the README. |
This is correct behavior:
Can you show the commands and versions that you think are broken now? |
I sorry I though the intended functionality was to make it work as it is in the README and as it was with the version I was using before. If that is the intended behavior the only thing that needs to be updated it the README. And some additional instructions. If Here's what happens now. This is similar to the behavior that broke Essentially when using
And
This seems much cleaner (though does not fix the problem encountered when using it more than once in a row!)
|
I used the prerelease name as the easiest solution. The
should not be happening. on my testing I get
so im not sure why yours is different. You have some interesting git shas getting in there though. Im mixed about the createTag because you can push prerelease tags to npm and they wont show up in the official list but do get pushed if you add the flag to get prerelease versions. Running bump:git multiple times without commiting any changes between should basically do nothing since nothing has changed. |
Seems top be a difference in your git describe options? Though I'm not entirely sure. I do know bump:git is broken for us on this version and on 0.2.0 was behaving as (I expected and as) the README indicated. Would you be against a PR that added a bump:gitDescribe option I mentioned? That (setting the version to the output of git describe) which would reinstate functionality that existed in v.0.0.11(even though I now see you thought it was a bug, we used it as a feature) Additionally the code I posted above does seem a lot cleaner way to do what you've got now, but probably no need for a refactor PR? README needs to be updated to indicate that bump:git is actually intended to bump the patch version. |
I think any bump should bump the version to at least the next patch release. Yeah the README should be updated to indicate that. You could always use the setVersion option and pass in a git describe block. Can you explain what your use case is for the "broken" way? I'm not quite grasping what you were doing. |
Sure: re: the existing In your example above on the 1st bump:git the it should be tagged
That would only make sense is the git describe portion was just I'd be happy to update the README but first I'd like to understand what's happening with bump:git. I would otherwise be writing documentation for behavior I am unable to replicate. I still maintain it is broken on the current version (now that I know what it's intended behavior is). I do not get the results you are showing above and don't see how, reading the code, that's what you're getting. |
https://github.com/vojtajina/grunt-bump/blob/master/tasks/bump.js#L120 takes the parsed version: Sorry, in the examples above I was using bump-only:git. Without the -only part then yes the sha1 (and version) should change between bumps since there are new commits. Although with type set to |
What do you get for git describe? Unless your git describe is somehow only the sha then your results are not consistent with what should be happening. In your example, somehow your prerease identifier (result from git describe) is only the sha and not the other parts of git decsribe which for me is |
Ok I was sort of wrong and sort of right git describe --tags --always --abbrev=1 --dirty=-d
----------------------------
v0.3.0
~/Sites/OSS/grunt-bump ⮀ GIT ⭠ master ● ⮀
grunt bump-only:git
-----------------------------
Running "bump-only:git" (bump-only) task
Running "bump:git:bump-only" (bump) task
>> Version bumped to 0.3.1-v0.3.0.0 (in package.json)
Done, without errors.
~/Sites/OSS/grunt-bump ⮀ GIT ⭠ master ● ⮀
grunt bump-only:git
-----------------------------
Running "bump-only:git" (bump-only) task
Running "bump:git:bump-only" (bump) task
>> Version bumped to 0.3.2-v0.3.0-d.0 (in package.json)
Done, without errors.
~/Sites/OSS/grunt-bump ⮀ GIT ⭠ master ● ⮀
grunt bump-only:git
-----------------------------
Running "bump-only:git" (bump-only) task
Running "bump:git:bump-only" (bump) task
>> Version bumped to 0.3.3-v0.3.0-d.0 (in package.json)
Done, without errors.
~/Sites/OSS/grunt-bump ⮀ GIT ⭠ master ● ⮀
grunt bump-only:git
-----------------------------
Running "bump-only:git" (bump-only) task
Running "bump:git:bump-only" (bump) task
>> Version bumped to 0.3.4-v0.3.0-d.0 (in package.json)
Done, without errors. |
Yea that looks closer to what I would expect and am experiencing. If you committed instead of Perhaps the better would to be use Thoughts? |
That sounds good. |
This fixes the issue where bump:git was not working when there were multiple files being bumped.
It also bumps the version of semver to allow for
Also adds bump:release to drop the pre-release portion of a version number.
e.g. from version 1.0.1-rc2 bump:release bumps the version from the prerelease version to 1.0.1.