Skip to content

Commit

Permalink
Added number support to version text formatter (#1620)
Browse files Browse the repository at this point in the history
  • Loading branch information
PyvesB authored and paulmelnikow committed Mar 29, 2018
1 parent 16eea6d commit 1dff491
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 0 deletions.
1 change: 1 addition & 0 deletions lib/text-formatters.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ function omitv(version) {
// - it is a date (yyyy-mm-dd)
const ignoredVersionPatterns = /^[^0-9]|[0-9]{4}-[0-9]{2}-[0-9]{2}/;
function addv(version) {
version = '' + version;
if (version.startsWith('v') || ignoredVersionPatterns.test(version)) {
return version;
} else {
Expand Down
2 changes: 2 additions & 0 deletions lib/text-formatters.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ describe('Text formatters', function() {
});

test(addv, () => {
given(9).expect('v9');
given(0.1).expect('v0.1');
given('1.0.0').expect('v1.0.0');
given('v0.6').expect('v0.6');
given('hello').expect('hello');
Expand Down

4 comments on commit 1dff491

@perceptron8
Copy link

@perceptron8 perceptron8 commented on 1dff491 Mar 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about 0.10?

@paulmelnikow
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In JSON, 0.10 can’t be represented as a number. It would need to be a string.

@perceptron8
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. That's why taking number as an argument makes no sense. Even if you argue that version can be a number (most software I know uses string however), it shouldn't be floating point number.

That' just another dirty hack. Why not just require string?

@perceptron8
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's more... According to JSON spec, limits on the range and precision of numbers accepted is an implementation detail (https://tools.ietf.org/html/rfc7159#section-6).

It's not like 0.10 can't be represented as a number in JSON. It could be. It can't be represented as a Number in JS runtime (it's represented approximately using IEEE 754; and it's also indistinguishable from 0.1).

You will do whatever you wan't. I just believe it's not very safe to allow numbers here and decided to let you know.

Please sign in to comment.