Consistent version formatting #1246

PyvesB · 2017-11-02T21:56:30Z

Hello,

This is a pull request related to issue #1181. It aims at providing greater consistency for badges related to versions. It's a slightly ambitious pull request, I'll try to give a clear outline of what was done:

the version function in color-formatters was previously returning the colour of the badge, but also its text with a leading v. It was broken down into two separate functions and the text formatting part was moved to text-formatters, where it really belongs.
unit tests were added for these two functions in color-formatters.spec and text-formatters.spec, using Sazerac.
as discussed in Leading "v" is forcefully added in github release badges #1181, the leading v was omitted for xxxx-yy-zz date patterns. Any future exceptions can easily be added to the ignoredVersionPatterns pattern.
the badge colour was previously switched to orange if a hyphen was found in the version string. This didn't seem ideal, instead pattern matching is done to find keywords such as beta, alpha or snapshot. Of course, this list can easily be extended.
all badges related to versions now use the versionText and versionColor functions. There are a few rare exceptions, for instance in cases where the data returned by the service's API allows to figure things out without relying on any parsing/pattern matching (eg. badgeData.colorscheme = prerelease ? 'orange' : 'blue';, where prerelease is determined from an API's response).

As always, looking forward to feedback! ☺️

Cheers,

Pyves

paulmelnikow · 2017-11-03T10:33:27Z

Nice! Looking forward to giving this a closer read.

tooomm · 2017-11-04T11:41:11Z

as discussed in Leading "v" is forcefully added in github release badges #1181, the leading v was omitted for xxxx-yy-zz date patterns. Any future exceptions can easily be added to the ignoredVersionPatterns pattern.

@paulmelnikow said there: "I'd probably start by approaching this the other way, and matching version types that look terrible with a v. For example, anything that starts with a xxxx-yy-zz, which is clearly a date, or anything which does not start with a number."
Is that second part covered here already?

the badge colour was previously switched to orange if a hyphen was found in the version string. This didn't seem ideal, instead pattern matching is done to find keywords such as beta, alpha or snapshot. Of course, this list can easily be extended.

development, prerelease and pre-release might be suitable keywords too

PyvesB · 2017-11-04T12:17:44Z

Is that second part covered here already?

It was already covered previously, and I have not altered that behaviour.

development, prerelease and pre-release might be suitable keywords too

They could easily be added. Nevertheless, are they often used in version numbers?

tooomm · 2017-11-05T12:13:14Z

Nevertheless, are they often used in version numbers?

Stumbled upon this Facebook repo on random in the dependency graph:
https://github.com/facebookarchive/esprima/releases

PyvesB · 2017-11-05T15:21:27Z

I have added the dev and pre keywords as matchers as well, that should do the trick.

paulmelnikow

Looks good!

paulmelnikow · 2017-11-09T21:50:37Z

lib/text-formatters.js

@@ -63,6 +63,17 @@ function omitv(version) {
  return version;
 }

+// Add a starting v to the version number if needed.
+const ignoredVersionPatterns = /^[^0-9]+|[0-9]{4}-[0-9]{2}-[0-9]{2}/;


^[^0-9]+ <- the trailing + is not needed.

Could you comment here that [0-9]{4}-[0-9]{2}-[0-9]{2} is to catch yyyy-mm-dd? It's clear from the tests, but it still helps here to understand the code.

paulmelnikow · 2017-11-09T21:54:49Z

lib/text-formatters.js

@@ -63,6 +63,17 @@ function omitv(version) {
  return version;
 }

+// Add a starting v to the version number if needed.
+const ignoredVersionPatterns = /^[^0-9]+|[0-9]{4}-[0-9]{2}-[0-9]{2}/;
+function addv(version) {


Double negatives are hard to understand, so when it's possible to rewrite an if/else to eliminate them, I tend to do that. Mercifully, we now have String#startsWith. I think this is a bit clearer that way.

if (version.startsWith('v') || ignoredVersionPatterns.test(version)) { return version; } else { return `v${version}`; }

Good point, I should have been more clever than just copy pasting the leftovers of the old function. ^^

paulmelnikow · 2017-11-09T21:55:42Z

lib/color-formatters.js

@@ -1,6 +1,6 @@
 /**
 * Commonly-used functions for determining the colour to use for a badge,
- * including colours based off download count, version number, etc.
+ * including colours based off download count, version colour, etc.


Not sure why this was changed. It seems like it was more grammatical before.

I must have misread the first part of the sentence... Will revert this!

paulmelnikow · 2017-11-09T21:55:55Z

lib/color-formatters.spec.js

+      given('6.0-SNAPSHOT'),
+      given('1.0.1-dev'),
+      given('2.1.6-prerelease'),
+      ]).expect('orange');


paulmelnikow · 2017-11-09T21:57:20Z

service-tests/nexus.js

@@ -30,7 +30,7 @@ t.create('search snapshot version not in latestSnapshot')
    .get('/nexus/service/local/lucene/search')
    .query({g: 'com.progress.fuse', a: 'fusehq'})
    .reply(200, '{ "data": [ { "version": "7.0.1-SNAPSHOT" } ] }'))
-  .expectJSON({ name: 'nexus', value: '7.0.1-SNAPSHOT' });
+  .expectJSON({ name: 'nexus', value: 'v7.0.1-SNAPSHOT' });


I wonder if we should omit v's on SNAPSHOT versions. Thoughts?

A quick search in Google shows that quite a few people tend to add a leading v with SNAPSHOT versions. Almost certainly not a majority, but I would be inclined to say we should add it and also be consistent with other keywords such as "BETA", "APLHA", etc. But I'm happy either way. 😉

I'm fine with leaving them in. We can see what people say once it's changed!

paulmelnikow · 2017-11-09T22:07:27Z

lib/color-formatters.js

  } else {
-    return { version: version, color: 'blue' };
+    return 'blue';


Thank you for separating these responsibilities. 🍻

paulmelnikow · 2017-11-10T02:39:23Z

lib/text-formatters.js

-const ignoredVersionPatterns = /^[^0-9]+|[0-9]{4}-[0-9]{2}-[0-9]{2}/;
+// Add a starting v to the version unless:
+// - it does not start with a digit
+// - it is a date (yyyy-mm-dd)


paulmelnikow · 2017-11-10T02:41:06Z

Do you feel like I should give all of server.js another once-over relative to this change? Or are you comfortable with it as it is?

PyvesB · 2017-11-10T09:00:55Z

Feel free to do so. But as you said "We can see what people say once it's changed!", and quickly iterate if they are unhappy about some of the changes. 😉

paulmelnikow · 2017-11-11T22:53:17Z

A few service tests failing locally, but nothing related to this change.

paulmelnikow · 2017-11-11T22:55:42Z

Thanks so much for this!

There might be a little bit of cleanup to do. For example, #1268 which was merged just before this, prepends a 'v'.

PyvesB · 2017-11-11T23:11:53Z

I'll go through recent changes and submit a small pull request to clean up anything that needs to be.

PyvesB added 3 commits November 2, 2017 21:18

Reworked the version formatters

fa4be71

Switched to const for never reassigned variable

1846d44

Fixed Sazerac syntax

1839b16

paulmelnikow added core Server, BaseService, GitHub auth, Shared helpers service-badge New or updated service badge labels Nov 3, 2017

Added dev and pre keyword matches

d454992

paulmelnikow reviewed Nov 9, 2017

View reviewed changes

PyvesB added 2 commits November 9, 2017 22:44

Fixed comment mess-up

6236719

Improved addv function

cf1b7d9

paulmelnikow reviewed Nov 10, 2017

View reviewed changes

paulmelnikow merged commit 7039e68 into badges:master Nov 11, 2017

PyvesB mentioned this pull request Nov 12, 2017

[elm-package] Cleaned up Elm Package version text and colour #1271

Merged

paulmelnikow mentioned this pull request Nov 21, 2017

Visual Studio Marketplace version badge should be blue like all the other version badges #1283

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consistent version formatting #1246

Consistent version formatting #1246

PyvesB commented Nov 2, 2017

paulmelnikow commented Nov 3, 2017

tooomm commented Nov 4, 2017

PyvesB commented Nov 4, 2017

tooomm commented Nov 5, 2017

PyvesB commented Nov 5, 2017

paulmelnikow left a comment

paulmelnikow Nov 9, 2017

paulmelnikow Nov 9, 2017

PyvesB Nov 9, 2017

paulmelnikow Nov 9, 2017

PyvesB Nov 9, 2017

paulmelnikow Nov 9, 2017

paulmelnikow Nov 9, 2017

PyvesB Nov 9, 2017 •

edited

Loading

paulmelnikow Nov 10, 2017

paulmelnikow Nov 9, 2017

paulmelnikow Nov 10, 2017

paulmelnikow commented Nov 10, 2017

PyvesB commented Nov 10, 2017

paulmelnikow commented Nov 11, 2017

paulmelnikow commented Nov 11, 2017

PyvesB commented Nov 11, 2017

Consistent version formatting #1246

Consistent version formatting #1246

Conversation

PyvesB commented Nov 2, 2017

paulmelnikow commented Nov 3, 2017

tooomm commented Nov 4, 2017

PyvesB commented Nov 4, 2017

tooomm commented Nov 5, 2017

PyvesB commented Nov 5, 2017

paulmelnikow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PyvesB Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmelnikow commented Nov 10, 2017

PyvesB commented Nov 10, 2017

paulmelnikow commented Nov 11, 2017

paulmelnikow commented Nov 11, 2017

PyvesB commented Nov 11, 2017

PyvesB Nov 9, 2017 •

edited

Loading