Repository: badges/shields
Pull request #3094 (2019-02-24 02:51:06)

Refactor [luarocks]

shields-ci: 
<!--
  0 failure: 
  0 warning: 
  1 messages
  
  DangerID: danger-id-default;
-->



<table>
  <thead>
    <tr>
      <th width="50"></th>
      <th width="100%" data-danger-table="true">Messages</th>
    </tr>
  </thead>
  <tbody><tr>
      <td>:book:</td>
      <td>:sparkles: Thanks for your contribution to Shields, @paulmelnikow!</td>
    </tr>
  </tbody>
</table>


<p align="right">
  Generated by :no_entry_sign: <a href="https://danger.systems/js">dangerJS</a> against e3f4f3c22f11f5182579920f064d3fc56d218bf6
</p>

paulmelnikow: > To my eye there's some non-trivial, domain specific content we have here (the version calcs, the version prefixes used for coloring in `render`, etc.). It's all well tested, but if there's any supporting docs/content we could reference with links in comments I think that'd be helpful for future observers

Yea, it's true. That code came in as part of a year-plus-long saga: #766

Hi @un-def 👋, would you happen to have any references we could link to?

Patch:
diff --git a/services/luarocks/luarocks-version.js b/services/luarocks/luarocks-version-helpers.js
similarity index 85%
rename from services/luarocks/luarocks-version.js
rename to services/luarocks/luarocks-version-helpers.js
index 6290a6a8cd..8ebc42d0e3 100644
--- a/services/luarocks/luarocks-version.js
+++ b/services/luarocks/luarocks-version-helpers.js
@@ -5,6 +5,8 @@
  */
 'use strict'
 
+const { omitv } = require('../../lib/text-formatters')
+
 // Compare two arrays containing split and transformed to
 // positive/negative numbers parts of version strings,
 // respecting negative/missing values:
@@ -23,7 +25,6 @@ function compareVersionLists(v1, v2) {
   }
   return 0
 }
-exports.compareVersionLists = compareVersionLists
 
 // Parse a dotted version string to an array of numbers
 // 'rc', 'pre', 'beta', 'alpha' are converted to negative numbers
@@ -56,4 +57,16 @@ function parseVersion(versionString) {
   })
   return versionList
 }
-exports.parseVersion = parseVersion
+
+function latestVersion(versions) {
+  return versions
+    .map(omitv)
+    .sort((a, b) => compareVersionLists(parseVersion(a), parseVersion(b)))
+    .pop()
+}
+
+module.exports = {
+  parseVersion,
+  compareVersionLists,
+  latestVersion,
+}
diff --git a/services/luarocks/luarocks-version.spec.js b/services/luarocks/luarocks-version-helpers.spec.js
similarity index 84%
rename from services/luarocks/luarocks-version.spec.js
rename to services/luarocks/luarocks-version-helpers.spec.js
index 1cce5bb219..95478d6fa1 100644
--- a/services/luarocks/luarocks-version.spec.js
+++ b/services/luarocks/luarocks-version-helpers.spec.js
@@ -1,7 +1,11 @@
 'use strict'
 
 const { test, given, forCases } = require('sazerac')
-const { parseVersion, compareVersionLists } = require('./luarocks-version')
+const {
+  parseVersion,
+  compareVersionLists,
+  latestVersion,
+} = require('./luarocks-version-helpers')
 
 describe('LuaRocks-specific helpers', function() {
   test(compareVersionLists, () => {
@@ -50,4 +54,9 @@ describe('LuaRocks-specific helpers', function() {
     given('2.0-beta').expect([2, 0, -3000])
     given('2.0-beta5').expect([2, 0, -2995])
   })
+
+  test(latestVersion, () => {
+    given(['1.2.4-3', '1.2.4-4', '1.2.6-1']).expect('1.2.6-1')
+    given(['1.2.4-3']).expect('1.2.4-3')
+  })
 })
diff --git a/services/luarocks/luarocks.service.js b/services/luarocks/luarocks.service.js
index 67a8fe00f8..abd97b810f 100644
--- a/services/luarocks/luarocks.service.js
+++ b/services/luarocks/luarocks.service.js
@@ -1,20 +1,20 @@
 'use strict'
 
-const LegacyService = require('../legacy-service')
-const { makeBadgeData: getBadgeData } = require('../../lib/badge-data')
-const { omitv, addv: versionText } = require('../../lib/text-formatters')
-const {
-  parseVersion: luarocksParseVersion,
-  compareVersionLists: luarocksCompareVersionLists,
-} = require('./luarocks-version')
+const Joi = require('joi')
+const { BaseJsonService, NotFound } = require('..')
+const { addv } = require('../../lib/text-formatters')
+const { latestVersion } = require('./luarocks-version-helpers')
 
-// This legacy service should be rewritten to use e.g. BaseJsonService.
-//
-// Tips for rewriting:
-// https://github.com/badges/shields/blob/master/doc/rewriting-services.md
-//
-// Do not base new services on this code.
-module.exports = class Luarocks extends LegacyService {
+const schema = Joi.object({
+  repository: Joi.object()
+    .pattern(
+      Joi.string(),
+      Joi.object().pattern(Joi.string(), Joi.array().strip())
+    )
+    .required(),
+}).required()
+
+module.exports = class Luarocks extends BaseJsonService {
   static get category() {
     return 'version'
   }
@@ -22,7 +22,7 @@ module.exports = class Luarocks extends LegacyService {
   static get route() {
     return {
       base: 'luarocks/v',
-      pattern: ':user/:moduleName',
+      pattern: ':user/:moduleName/:version?',
     }
   }
 
@@ -34,80 +34,64 @@ module.exports = class Luarocks extends LegacyService {
           user: 'mpeterv',
           moduleName: 'luacheck',
         },
-        staticPreview: {
-          label: 'luarocks',
-          message: 'v0.23.0-1',
-          color: 'brightgreen',
-        },
+        staticPreview: this.render({ version: '0.23.0-1' }),
       },
     ]
   }
 
-  static registerLegacyRouteHandler({ camp, cache }) {
-    camp.route(
-      /^\/luarocks\/v\/([^/]+)\/([^/]+)(?:\/(.+))?\.(svg|png|gif|jpg|json)$/,
-      cache((data, match, sendBadge, request) => {
-        const user = match[1] // eg, `leafo`.
-        const moduleName = match[2] // eg, `lapis`.
-        const format = match[4]
-        const apiUrl = `https://luarocks.org/manifests/${user}/manifest.json`
-        const badgeData = getBadgeData('luarocks', data)
-        let version = match[3] // you can explicitly specify a version
-        request(apiUrl, (err, res, buffer) => {
-          if (err != null) {
-            badgeData.text[1] = 'inaccessible'
-            sendBadge(format, badgeData)
-            return
-          }
-          let versions
-          try {
-            const moduleInfo = JSON.parse(buffer).repository[moduleName]
-            versions = Object.keys(moduleInfo)
-            if (version && versions.indexOf(version) === -1) {
-              throw new Error('unknown version')
-            }
-          } catch (e) {
-            badgeData.text[1] = 'invalid'
-            sendBadge(format, badgeData)
-            return
-          }
-          if (!version) {
-            if (versions.length === 1) {
-              version = omitv(versions[0])
-            } else {
-              let latestVersionString, latestVersionList
-              versions.forEach(versionString => {
-                versionString = omitv(versionString) // remove leading 'v'
-                const versionList = luarocksParseVersion(versionString)
-                if (
-                  !latestVersionList || // first iteration
-                  luarocksCompareVersionLists(versionList, latestVersionList) >
-                    0
-                ) {
-                  latestVersionString = versionString
-                  latestVersionList = versionList
-                }
-              })
-              version = latestVersionString
-            }
-          }
-          let color
-          switch (version.slice(0, 3).toLowerCase()) {
-            case 'dev':
-              color = 'yellow'
-              break
-            case 'scm':
-            case 'cvs':
-              color = 'orange'
-              break
-            default:
-              color = 'brightgreen'
-          }
-          badgeData.text[1] = versionText(version)
-          badgeData.colorscheme = color
-          sendBadge(format, badgeData)
-        })
-      })
-    )
+  async fetch({ user, moduleName }) {
+    const { repository } = await this._requestJson({
+      url: `https://luarocks.org/manifests/${encodeURIComponent(
+        user
+      )}/manifest.json`,
+      schema,
+      errorMessages: {
+        404: 'user not found',
+      },
+    })
+    const moduleData = repository[moduleName]
+    if (!moduleData) {
+      throw new NotFound({ prettyMessage: 'module not found' })
+    }
+    return moduleData
+  }
+
+  static get defaultBadgeData() {
+    return {
+      label: 'luarocks',
+    }
+  }
+
+  static render({ version }) {
+    let color
+    switch (version.slice(0, 3).toLowerCase()) {
+      case 'dev':
+        color = 'yellow'
+        break
+      case 'scm':
+      case 'cvs':
+        color = 'orange'
+        break
+      default:
+        color = 'brightgreen'
+    }
+
+    return { message: addv(version), color }
+  }
+
+  async handle({ user, moduleName, version: requestedVersion }) {
+    const moduleInfo = await this.fetch({ user, moduleName })
+
+    let version
+    if (requestedVersion) {
+      if (!(requestedVersion in moduleInfo)) {
+        throw new NotFound({ prettyMessage: 'version not found' })
+      }
+      version = requestedVersion
+    } else {
+      const versions = Object.keys(moduleInfo)
+      version = latestVersion(versions)
+    }
+    return this.constructor.render({ version })
   }
 }
diff --git a/services/luarocks/luarocks.spec.js b/services/luarocks/luarocks.spec.js
new file mode 100644
index 0000000000..ad6479ef7d
--- /dev/null
+++ b/services/luarocks/luarocks.spec.js
@@ -0,0 +1,14 @@
+'use strict'
+
+const { test, given } = require('sazerac')
+const Luarocks = require('./luarocks.service')
+
+test(Luarocks.render, () => {
+  given({ version: 'dev-1' }).expect({ message: 'dev-1', color: 'yellow' })
+  given({ version: 'scm-1' }).expect({ message: 'scm-1', color: 'orange' })
+  given({ version: 'cvs-1' }).expect({ message: 'cvs-1', color: 'orange' })
+  given({ version: '0.1-1' }).expect({
+    message: 'v0.1-1',
+    color: 'brightgreen',
+  })
+})


Test patch:
diff --git a/services/luarocks/luarocks.tester.js b/services/luarocks/luarocks.tester.js
index 00c083ba69..aebade77a8 100644
--- a/services/luarocks/luarocks.tester.js
+++ b/services/luarocks/luarocks.tester.js
@@ -1,15 +1,14 @@
 'use strict'
 
 const Joi = require('joi')
-const { ServiceTester } = require('../tester')
+const t = (module.exports = require('../tester').createServiceTester())
 
-const t = new ServiceTester({ id: 'luarocks', title: 'LuaRocks' })
-module.exports = t
-
-const isLuaVersion = Joi.string().regex(/^v\d+\.\d+\.\d+-\d+$/)
+const isLuaVersion = Joi.string()
+  .regex(/^v\d+\.\d+\.\d+-\d+$/)
+  .required()
 
 t.create('version')
-  .get('/v/mpeterv/luacheck.json')
+  .get('/mpeterv/luacheck.json')
   .expectJSONTypes(
     Joi.object().keys({
       name: 'luarocks',
@@ -17,11 +16,18 @@ t.create('version')
     })
   )
 
-t.create('unknown package')
-  .get('/v/nil/does-not-exist.json')
-  .expectJSON({ name: 'luarocks', value: 'invalid' })
+t.create('specified version')
+  .get('/mpeterv/luacheck/0.9.0-1.json')
+  .expectJSON({ name: 'luarocks', value: 'v0.9.0-1' })
+
+t.create('unknown version')
+  .get('/mpeterv/luacheck/0.0.0.json')
+  .expectJSON({ name: 'luarocks', value: 'version not found' })
+
+t.create('unknown module')
+  .get('/mpeterv/does-not-exist.json')
+  .expectJSON({ name: 'luarocks', value: 'module not found' })
 
-t.create('connection error')
-  .get('/v/mpeterv/luacheck.json')
-  .networkOff()
-  .expectJSON({ name: 'luarocks', value: 'inaccessible' })
+t.create('unknown user')
+  .get('/nil/does-not-exist.json')
+  .expectJSON({ name: 'luarocks', value: 'user not found' })