Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[gem cdnjs appveyor clojars] refactor clojars, establish BaseJsonService #1702

Merged
merged 4 commits into from
Jun 16, 2018
Merged

[gem cdnjs appveyor clojars] refactor clojars, establish BaseJsonService #1702

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
056ce44
refactor clojars integration
chris48s May 25, 2018
779df45
DRY up services that request data from JSON endpoints
chris48s May 25, 2018
fb40893
fix whitespace
chris48s Jun 14, 2018
3904597
don't allow empty string params
chris48s Jun 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions lib/all-badge-examples.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,10 +885,6 @@ const allBadgeExamples = [
title: 'Maven metadata URI',
previewUri: '/maven-metadata/v/http/central.maven.org/maven2/com/google/code/gson/gson/maven-metadata.xml.svg'
},
{
title: 'Clojars',
previewUri: '/clojars/v/prismic.svg'
},
{
title: 'CocoaPods',
previewUri: '/cocoapods/v/AFNetworking.svg'
Expand Down
32 changes: 0 additions & 32 deletions server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2064,38 +2064,6 @@ cache(function(data, match, sendBadge, request) {
});
}));

// Clojars version integration
camp.route(/^\/clojars\/v\/(.+)\.(svg|png|gif|jpg|json)$/,
cache(function(data, match, sendBadge, request) {
const clojar = match[1]; // eg, `prismic` or `foo/bar`.
const format = match[2];
const apiUrl = 'https://clojars.org/' + clojar + '/latest-version.json';
const badgeData = getBadgeData('clojars', data);
request(apiUrl, function(err, res, buffer) {
if (err !== null) {
badgeData.text[1] = 'inaccessible';
sendBadge(format, badgeData);
return;
}
try {
const json = JSON.parse(buffer);
if (Object.keys(json).length === 0) {
/* Note the 'not found' response from clojars is:
status code = 200, body = {} */
badgeData.text[1] = 'not found';
sendBadge(format, badgeData);
return;
}
badgeData.text[1] = "[" + clojar + " \"" + json.version + "\"]";
badgeData.colorscheme = versionColor(json.version);
sendBadge(format, badgeData);
} catch(e) {
badgeData.text[1] = 'invalid';
sendBadge(format, badgeData);
}
});
}));

// iTunes App Store version
camp.route(/^\/itunes\/v\/(.+)\.(svg|png|gif|jpg|json)$/,
cache(function(data, match, sendBadge, request) {
Expand Down
13 changes: 3 additions & 10 deletions services/appveyor/appveyor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,14 @@
'use strict';

const BaseService = require('../base');
const {
checkErrorResponse,
asJson,
} = require('../../lib/error-helper');
const { BaseJsonService } = require('../base');

module.exports = class AppVeyor extends BaseService {
module.exports = class AppVeyor extends BaseJsonService {
async handle({repo, branch}) {
let apiUrl = 'https://ci.appveyor.com/api/projects/' + repo;
if (branch != null) {
apiUrl += '/branch/' + branch;
}
const json = await this._sendAndCacheRequest(apiUrl, {
headers: { 'Accept': 'application/json' }
}).then(checkErrorResponse.asPromise({ notFoundMessage: 'project not found or access denied' }))
.then(asJson);
const json = await this._requestJson(apiUrl, {}, 'project not found or access denied');

const { build: { status } } = json;
if (status === 'success') {
Expand Down
24 changes: 23 additions & 1 deletion services/base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,13 @@ const {
makeColor,
setBadgeColor,
} = require('../lib/badge-data');
const {
checkErrorResponse,
asJson,
} = require('../lib/error-helper');


module.exports = class BaseService {
class BaseService {
constructor({ sendAndCacheRequest }, { handleInternalErrors }) {
this._sendAndCacheRequest = sendAndCacheRequest;
this._handleInternalErrors = handleInternalErrors;
Expand Down Expand Up @@ -210,3 +215,20 @@ module.exports = class BaseService {
}));
}
};

class BaseJsonService extends BaseService {
async _requestJson(url, options = {}, notFoundMessage) {
return this._sendAndCacheRequest(url,
{...{ 'headers': { 'Accept': 'application/json' } }, ...options}
).then(
checkErrorResponse.asPromise(
notFoundMessage ? { notFoundMessage: notFoundMessage } : undefined
)
).then(asJson);
}
};

module.exports = {
BaseService,
BaseJsonService,
};
2 changes: 1 addition & 1 deletion services/base.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const { expect } = require('chai');
const { test, given, forCases } = require('sazerac');
const sinon = require('sinon');

const BaseService = require('./base');
const { BaseService } = require('./base');

require('../lib/register-chai-plugins.spec');

Expand Down
13 changes: 3 additions & 10 deletions services/cdnjs/cdnjs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,14 @@
'use strict';

const BaseService = require('../base');
const {
checkErrorResponse,
asJson,
} = require('../../lib/error-helper');
const { BaseJsonService } = require('../base');
const { NotFound } = require('../errors');
const { addv: versionText } = require('../../lib/text-formatters');
const { version: versionColor} = require('../../lib/color-formatters');

module.exports = class Cdnjs extends BaseService {
module.exports = class Cdnjs extends BaseJsonService {
async handle({library}) {
const apiUrl = 'https://api.cdnjs.com/libraries/' + library + '?fields=version';
const json = await this._sendAndCacheRequest(apiUrl, {
headers: { 'Accept': 'application/json' }
}).then(checkErrorResponse.asPromise())
.then(asJson);
const json = await this._requestJson(apiUrl);

if (Object.keys(json).length === 0) {
/* Note the 'not found' response from cdnjs is:
Expand Down
47 changes: 47 additions & 0 deletions services/clojars/clojars.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
'use strict';

const { BaseJsonService } = require('../base');
const { NotFound } = require('../errors');
const { version: versionColor} = require('../../lib/color-formatters');
Copy link
Member

Choose a reason for hiding this comment

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

Being picky: inconsistent spacing around versionColor ^^

Copy link
Member

Choose a reason for hiding this comment

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

Surprised we don't have an eslint rule for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. I was also surprised this isn't caught by a lint rule. Looks like we would need to configure ESLint's object-curly-spacing rule. If we want to add a convention for this, there's going to be a bunch of other code knocking about that doesn't conform to it so lets do it in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not that familiar with ESLint, but I'm happy to take care of this and dig into it some more. 😉

Copy link
Member

Choose a reason for hiding this comment

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

Actually, after taking a closer look into this, we do have a rule enabled in Prettier. Nevertheless, it's currently awaiting to be enabled as part of #1167.


module.exports = class Clojars extends BaseJsonService {
async handle({clojar}) {
const apiUrl = 'https://clojars.org/' + clojar + '/latest-version.json';
const json = await this._requestJson(apiUrl);

if (Object.keys(json).length === 0) {
/* Note the 'not found' response from clojars is:
status code = 200, body = {} */
throw new NotFound();
}

return {
message: "[" + clojar + " \"" + json.version + "\"]",
color: versionColor(json.version)
};
}

// Metadata
static get defaultBadgeData() {
return { label: 'clojars' };
}

static get category() {
return 'version';
}

static get url() {
return {
base: 'clojars/v',
format: '(.*)',
Copy link
Member

Choose a reason for hiding this comment

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

The old implementation used (.+), I would be inclined to keep it that way. It won't make that much of a difference, but we will avoid capturing an empty string and making a request that we know is incorrect to the Clojars service.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Good spot. I've also changed it in the other services I have touched in this PR as there is no case for empty string there either. I'll try to remember this as I refactor more services.

capture: ['clojar']
};
}

static get examples() {
return [
{ previewUrl: 'prismic' }
];
}

};
34 changes: 9 additions & 25 deletions services/gem/gem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,7 @@

const semver = require('semver');

const BaseService = require('../base');
const {
checkErrorResponse,
asJson,
} = require('../../lib/error-helper');
const { BaseJsonService } = require('../base');
const { InvalidResponse } = require('../errors');
const { addv: versionText } = require('../../lib/text-formatters');
const { version: versionColor} = require('../../lib/color-formatters');
Expand All @@ -21,13 +17,10 @@ const {
const { latest: latestVersion } = require('../../lib/version');


class GemVersion extends BaseService {
class GemVersion extends BaseJsonService {
async handle({repo}) {
const apiUrl = 'https://rubygems.org/api/v1/gems/' + repo + '.json';
const json = await this._sendAndCacheRequest(apiUrl, {
headers: { 'Accept': 'application/json' }
}).then(checkErrorResponse.asPromise())
.then(asJson);
const json = await this._requestJson(apiUrl);
const version = json.version;

return {
Expand Down Expand Up @@ -66,7 +59,7 @@ class GemVersion extends BaseService {
}
};

class GemDownloads extends BaseService {
class GemDownloads extends BaseJsonService {

_getApiUrl(repo, info) {
const endpoint = info === "dv" ? 'versions/' : 'gems/';
Expand Down Expand Up @@ -94,10 +87,7 @@ class GemDownloads extends BaseService {
version = (version === "stable") ? version : semver.valid(version);
const label = this._getLabel(version, info);
const apiUrl = this._getApiUrl(repo, info);
const json = await this._sendAndCacheRequest(apiUrl, {
headers: { 'Accept': 'application/atom+json,application/json' }
Copy link
Member

Choose a reason for hiding this comment

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

This particular line had a slightly different Accept header, any idea why?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was added in #212 but there is no explanation there. It works fine with the standard Accept: application/json it so I didn't see the need to cargo cult it.

}).then(checkErrorResponse.asPromise())
.then(asJson);
const json = await this._requestJson(apiUrl);

let downloads;
if (info === "dt") {
Expand Down Expand Up @@ -194,14 +184,11 @@ class GemDownloads extends BaseService {
}
};

class GemOwner extends BaseService {
class GemOwner extends BaseJsonService {

async handle({user}) {
const apiUrl = 'https://rubygems.org/api/v1/owners/' + user + '/gems.json';
const json = await this._sendAndCacheRequest(apiUrl, {
headers: { 'Accept': 'application/json' }
}).then(checkErrorResponse.asPromise())
.then(asJson);
const json = await this._requestJson(apiUrl);
const count = json.length;

return {
Expand Down Expand Up @@ -240,7 +227,7 @@ class GemOwner extends BaseService {
}
};

class GemRank extends BaseService {
class GemRank extends BaseJsonService {

_getApiUrl(repo, totalRank, dailyRank) {
let endpoint;
Expand All @@ -256,10 +243,7 @@ class GemRank extends BaseService {
const totalRank = (info === 'rt');
const dailyRank = (info === 'rd');
const apiUrl = this._getApiUrl(repo, totalRank, dailyRank);
const json = await this._sendAndCacheRequest(apiUrl, {
headers: { 'Accept': 'application/json' }
}).then(checkErrorResponse.asPromise())
.then(asJson);
const json = await this._requestJson(apiUrl);

let rank;
if (totalRank) {
Expand Down