tvdstaaij / node-git-describe

Git describe information at runtime, with semver support
36 stars 5 forks source link

[usability issue] Doesn't return either `distance` or `semver` #27

Open OnkelTem opened 2 years ago

OnkelTem commented 2 years ago

I created a simple js file in the root of my project:

const { gitDescribeSync } = require('git-describe')
const gitInfo = gitDescribeSync()
console.log(gitInfo)

which returns:

{
  dirty: true,
  raw: 'd6d7fc7-dirty',
  hash: 'd6d7fc7',
  distance: null,
  tag: null,
  semver: null,
  suffix: 'd6d7fc7-dirty',
  semverString: null,
  toString: [Function (anonymous)]
}

like it couldn't acquire either of those. But git describe w/o params properly returns:

0.4.1-4-gd6d7fc7
OnkelTem commented 2 years ago

Ok, I've resolved the issue after investigating source code. We don't use v prefix for our versions, while git-describe module uses --match option equal to 'v[0-9]*' which filtered our tags out.

I would recommend not using any mask by default.

TimothyJones commented 2 years ago

I think the reason for the default mask is that several popular tools produce versions in that format. It could perhaps be better highlighted, or intelligently fall back by default.

tvdstaaij commented 2 years ago

The reason is that this was initially developed for a specific use case, and the defaults were chosen to fit that particular use case. Publishing it for reuse by others was more of an afterthought. Over time I did add options and made some tweaks to make it more suitable for other use cases as well, but the defaults are not optimized for the broadest possible audience.

So basically it boils down to a legacy issue. I wouldn't mind changing this, but I don't think it's worth breaking compatibility over. Maybe it could be bundled if there is ever a more fundamental reason for a semver-major update. @TimothyJones I'm not sure how an intelligent fallback would work, do you have an idea of how that could be achieved without breaking compatibility with code that relies on this default?

TimothyJones commented 2 years ago

I think bringing in a fallback would be a breaking change. What I was thinking was it might be nice if the default was vX.Y.Z, and it fell back to X.Y.Z only if it didn't find anything, but I'm not actually sure that's a good idea - it's good if software just works, but it's not good if the behaviour is too surprising or might result in the wrong output.

I think the ideal might be a warning of "hey we didn't find anything, but we did find X.Y.Z without the prefix", but I'm not sure how that warning would best be presented - printing to standard error won't be what everyone wants, and throwing an error seems a bit harsh. We could add a debug option to print warnings, or make it throw and add a force option to make it ignore the warnings - but both those options feel more complex than the benefit we'd get.

I made the claim that v is a good default because it's the default prefix on other versioning tools, so I did a quick survey of the ones I'm familiar with:

All of these include the v prefix by default. There probably are tools that don't, but I don't personally know any.

The semver spec doesn't mention any prefix, but I think the reason the v prefix is the conventional default on tags is to make it clear that the tag is intended to represent a version.