tvdstaaij / node-git-describe

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

Type definition for match option should accept null/false value #20

Closed bobo96run closed 2 years ago

bobo96run commented 3 years ago

Hi,

I noticed that node-git-describe recently got built-in (i.e. directly provided by the package) TypeScript definitions, that is such a nice added feature!! 👍

While it helped me better use the library in several occasions, there is one specific situation at least where it seems to me that the types miss a use case: for the match option, I used to pass false boolean value to prevent any match from happening (no --match option used on git-describe command).

The current types accepts only a string value, which, in strict mode, does not even accept null. https://github.com/tvdstaaij/node-git-describe/blob/91dc7f184f6410b7cc6adc0bd203f0bc6bf0efcd/index.d.ts#L20

Then if I pass any string value, it will replace the default value (which matches anything looking like a version) https://github.com/tvdstaaij/node-git-describe/blob/91dc7f184f6410b7cc6adc0bd203f0bc6bf0efcd/lib/git-describe.js#L29 I can use the "*" wildcard, but this still tries to match a tag.

Whereas I used to pass it false to prevent any match attempt, which is correctly handled in that case in the JS code: https://github.com/tvdstaaij/node-git-describe/blob/91dc7f184f6410b7cc6adc0bd203f0bc6bf0efcd/lib/git-describe.js#L60-L61

Using match: undefined makes the TS error go away, but does not work with the JS code, because the default value is still used in that case (expected behavior of lodash.defaults)

At least when using match: null I can get the initial behavior, but this does not pass the current types.

Therefore it seems to me that it is an expected behavior to be able to specify null or false to the match option, when we want to disable the --match option of git-describe command, hence it should be added in the types?

bobo96run commented 3 years ago

Hum maybe I tested a little bit too quick, let me check again...

bobo96run commented 3 years ago

Ok so actually match: "*" does the same as no --match flag, so I guess we can still cover that use case with a string match value after all.