Draft release

[TimothyJones]

This PR fixes all of the currently open issues (#15, #16 and #17). I haven't done the release chores (version bump / changelog etc) yet, so that if you have any review comments we can fix those up first.

I've tried to match the implementation style in the repository. It's a little outside my usual style, which was quite fun. However, I might have missed the mark in places - please let me know if there are any idioms you would like to change. Of course, all other feedback is welcome too.

Some things to mention:

• While implementing the optional long (Make --long an optional arguement #15), I noticed that the behaviour of longSemver is slightly different to git describe --long when the repo is dirty. When longSemver: false on an exact tag, the semver is of the form 1.2.3-0.abce12.dirty (including the commit count and hash), whereas git describe without --long is just v1.2.3-dirty. I did not change the behaviour of longSemver to match --long, because I guess it's technically a breaking change. It's a pretty straightforward change if you do want to make it, though.
• I didn't migrate to typescript, I just manually created the types. It's probably better to actually migrate to typescript, because then the types will be autogenerated (and checked at compile time, etc). I've left this for future work, but I'd be happy to do it now if you prefer.
• The nodejs docs for spawnSync are actually wrong about the return type - it says stdout and stderr are <Buffer> | <string>, but they are both null if the process fails during spawn (at least they are on node 14). So I put in a guard for that.
• The issue I experienced when git was missing (Cannot read property 'toString' of null when git is missing #17) was only present in gitDescribeSync. Most of the tests only test gitDescribe. It might make sense to refactor the tests to include a harness that runs each test with both gitDescribe and gitDescribeSync. Again, I left this for future work, but I'd be happy to do it now if you prefer.

[tvdstaaij]

LGTM, I'll just trust you on the TypeScript definitions since I haven't used it at all :) Not that I'd mind this module being TypeScript, I've heard good things about it and would check it out sooner or later anyway.

I noticed that the behaviour of longSemver is slightly different to git describe --long when the repo is dirty. When longSemver: false on an exact tag, the semver is of the form 1.2.3-0.abce12.dirty (including the commit count and hash), whereas git describe without --long is just v1.2.3-dirty. I did not change the behaviour of longSemver to match --long, because I guess it's technically a breaking change. It's a pretty straightforward change if you do want to make it, though.

Good catch. Though I doubt anyone relies on this detail it is technically breaking, yeah. And having been screwed over by updates of modules where it was decided that something was "technically breaking but let's just pass it off as a feature/fix" a few times too often, I'd say it should be semver-major. Conversely I expect that the typical use case for this module isn't hindered by the current behavior either, so my suggestion is to leave it for now and incorporate it in a major release if other breaking changes are needed in the future, or until someone complains about it.

I've left this for future work, but I'd be happy to do it now if you prefer.
Again, I left this for future work, but I'd be happy to do it now if you prefer.

I don't see these as pressing issues either so I'd say this is good to go for release.

[TimothyJones]

Ok! I have added standard-version to make it easy to cut releases, added a contributing guide to explain how it works, and used it to bump the version and create an appropriate git tag.

I'm actually not 100% sure how a git tag behaves on a merge - I think it'll come across with the commits as long as the merge is a fast-forward merge, but this is outside the things I am certain about in git land.

I've also added some config in package.json which replaces the default sections in the changelog output from standard-version - the default changelog sections were Features and Bug Fixes. The config I have added swaps those sections for New Features and Fixes and Improvements respectively. I do this because not every fix that you want in the changelog is actually a "Bug Fix" - it's not a big deal, but I wanted to highlight it, since I'm introducing personal preferences over the default behaviour of a common tool.

[TimothyJones]

I just realised I didn't say this explicitly - this is now ready for merge and release, which I'll leave to you. Of course, you might want to run the tests first, since Travis isn't running the CI at the moment.

I'm planning to get to the travis -> github actions uplift sometime next week.

[tvdstaaij]

Alright, it's been published. Thanks for your efforts so far!

Of course, you might want to run the tests first, since Travis isn't running the CI at the moment.

I actually ended up discovering one minor bug in the tests, specifically when git is globally configured to use GPG signing. I'll fix that on master though since it wasn't worth breaking up the release over.

I'm actually not 100% sure how a git tag behaves on a merge - I think it'll come across with the commits as long as the merge is a fast-forward merge, but this is outside the things I am certain about in git land.

Yeah the tag does come along, provided the commits aren't squashed or anything like that. The tag is simply a pointer to a certain commit, so as long as the commit it points to stays intact there is no problem. (It can be argued that a tag on the merge commit itself is a bit "cleaner", but I don't think it's a big deal.)

I've also added some config in package.json which replaces the default sections in the changelog output from standard-version (...) it's not a big deal, but I wanted to highlight it, since I'm introducing personal preferences over the default behaviour of a common tool.

Sure, looks fine.

[cmeury]

Small side-note, the latest release here on GitHub is still 4.0.4.