[GitHub] [cordova-ios] breautek commented on a change in pull request #695: (iOS) Added Semver to version comparison checks.

[GitBox <gi...@apache.org>]

breautek commented on a change in pull request #695: (iOS) Added Semver to version comparison checks.
URL: https://github.com/apache/cordova-ios/pull/695#discussion_r337290493
 
 

 ##########
 File path: bin/templates/scripts/cordova/lib/versions.js
 ##########
 @@ -167,28 +168,20 @@ exports.get_tool_version = function (toolName) {
  *                                    positive otherwise and 0 if versions are equal.
  */
 exports.compareVersions = function (version1, version2) {
-    function parseVer (version) {
-        return version.split('.').map(function (value) {
-            // try to convert version segment to Number
-            var parsed = Number(value);
-            // Number constructor is strict enough and will return NaN
-            // if conversion fails. In this case we won't be able to compare versions properly
-            if (isNaN(parsed)) {
-                throw 'Version should contain only numbers and dots';
-            }
-            return parsed;
-        });
+    // coerce and validate versions
+    var cleanV1 = semver.valid(semver.coerce(version1));
+    var cleanV2 = semver.valid(semver.coerce(version2));
+
+    // throw exception in the event one or both versions cannot be validated
+    if (cleanV1 === null || cleanV2 === null) {
+        throw 'Version should be in valid semver syntax. See: https://semver.org/';
     }
-    var parsedVer1 = parseVer(version1);
-    var parsedVer2 = parseVer(version2);
-
-    // Compare corresponding segments of each version
-    for (var i = 0; i < Math.max(parsedVer1.length, parsedVer2.length); i++) {
-        // if segment is not specified, assume that it is 0
-        // E.g. 3.1 is equal to 3.1.0
-        var ret = (parsedVer1[i] || 0) - (parsedVer2[i] || 0);
-        // if segments are not equal, we're finished
-        if (ret !== 0) return ret;
+
+    // if versions are equivalent, return 0;
+    if (cleanV1 === cleanV2) {
 
 Review comment:
   `semver.coerce` drops labels.
   
   When `cleanV1` and `cleanV2` is equal, do you think it's worth checking the original `version` variables and determine if one has a prerelease label, then return the appropriate integer?
   
   e.g.
   
   ```javascript
   var version1 = "1.0.0";
   var version2 = "1.0.0-beta.0";
   
   var cleanV1 = semver.valid(semver.coerce(version1));
   var cleanV2 = semver.valid(semver.coerce(version2));
   
   if (cleanV1 === cleanV2) {
       if (version1.indexOf('-') > -1 && version2.indexOf('-') === -1) {
           // version1 is prerelease, favour version2 instead.
       }
       else if (version2.indexOf('-') > -1 && version1.indexOf('-') === -1) {
           //version 2 is prelease, favour version 1
       }
       else {
           return 0;
       }
   }

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org