-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
implement semver --increment to remove dependency #45
Conversation
"bundledDependencies": [ | ||
"semver" | ||
] | ||
"dependencies": {} |
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.
🎉
Specify the label to be used in the version number when publishing | ||
a pre-release version (e.g. 'beta' is the label in '2.0.0-beta.0'). | ||
'rc' is assumed if this option is omitted. | ||
|
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 addition shows that we have never fully supported the pre{major,minor,patch,release}
increments.
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.
@Avaq, do you agree with rc
as the default value?
path="$(readlink "$path")" | ||
[[ $path == /* ]] || path="$dir/$path" | ||
done | ||
dir="$(cd -P "$(dirname "$path")" && pwd)" |
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'm happy to see this code go!
local prerelease_label="$1" increment="$2" version="$3" | ||
local number='0|[1-9][0-9]*' | ||
local part="$number|[-0-9A-Za-z]+" | ||
local pattern="^($number)[.]($number)[.]($number)(-(($part)([.]($part))*))?$" |
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 should match the grammar.
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've missed the build
part. Not sure if that's really relevant though.
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've missed the
build
part.
That's true. I've never used it. Have you, Aldwin?
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've never used it either.
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.
Let's omit build
. I don't understand how it is used, and I'm not aware of xyz users actually using it. If in the future there is demand for build
, we're free to change our minds. :)
xyz
Outdated
patch) [[ -n $suff ]] || z+=1 ;; | ||
premajor) x+=1 ; y=0 ; z=0 ;; | ||
preminor) y+=1 ; z=0 ;; | ||
prepatch) z+=1 ;; |
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.
The majority of the algorithm is implemented in the six lines above. It reads better to me than the mutation-laden JavaScript implementation. ;)
@@ -90,29 +138,25 @@ while (($# > 0)) ; do | |||
exit | |||
;; | |||
-v|--version) | |||
node -p "require('$dir/package.json').version" | |||
node -p 'require("xyz/package.json").version' |
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 change means ./xyz --version
no longer works (from this project's root directory), but I don't consider this a problem. Not having to determine our location on the file system is a win.
major|minor|patch|premajor|preminor|prepatch|prerelease) ;; | ||
*) echo "Invalid --increment" >&2 ; exit 1 ;; | ||
esac | ||
|
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.
The --increment
value is validated in inc
, so this paragraph is no longer necessary.
I've wanted to do this for some time. The complexity of the code we add to the project by removing the dependency is offset by the incidental complexity we remove from the project by doing so. This change inadvertently closes #2 as we no longer need to determine our location on the file system.
We have no test suite, in part because I'd like to keep the source code in a single file which makes it difficult to test individual functions without triggering the script's side effects. I wrote a throwaway program to assert that the new
inc
function produces the same results assemver --increment
(usingsemver@5.3.0
) for 1792 inputs. Here's the test matrix I used:I think this covers all the edge cases. Please let me know if you think of one I missed. :)