Add [LGTM] service #1672
Add [LGTM] service #1672
Conversation
Generated by 🚫 dangerJS |
server.js
Outdated
| var projectId = match[1]; // eg, `g/apache/cloudstack` | ||
| var format = match[2]; | ||
| var url = 'https://lgtm.com/api/v0.1/project/' + projectId + '/details'; | ||
| var badgeData = getBadgeData('lgtm', data); |
There was a problem hiding this comment.
Could you use the ES6 style let and const (I know we have a lot of code using var, but we're trying to update as we add new code).
server.js
Outdated
| } | ||
| try { | ||
| var data = JSON.parse(buffer); | ||
| if (data.code === 404) { |
There was a problem hiding this comment.
Assuming when data.code === 404 the HTTP status code is also 404, this block here and the section above where you're checking for if (err != null) can all be simplified to:
if (checkErrorResponse(badgeData, err, res, 'project not found')) {
sendBadge(format, badgeData);
return;
}it is a very common check, so we have a helper for it
There was a problem hiding this comment.
It indeed is, i'll change it
| .get('/alerts/g/apache/cloudstack.json') | ||
| .expectJSONTypes(Joi.object().keys({ | ||
| name: 'lgtm', | ||
| value: Joi.string().regex(/^[0-9kM.]+ alerts?$/) |
There was a problem hiding this comment.
I think this regex should allow 'alert' or 'alerts' (i.e: it should pass however many alerts apache/cloudstack has when we run the test).
There was a problem hiding this comment.
Sorry - ignore this comment. I've just noticed it is already alerts?$ not alerts$.
My bad
There was a problem hiding this comment.
👍 In fairness, i did originally write it with out the ?, i'll add tests to make sure that it prints it our correctly for different cases with an intercept()
services/lgtm/lgtm.tester.js
Outdated
|
|
||
| t.create('missing project') | ||
| .get('/alerts/g/some-org/this-project-doesnt-exist.json') | ||
| .expectJSONTypes(Joi.object().keys({ |
There was a problem hiding this comment.
A bit of project convention: We use .expectJSON() if we're comparing the response to an object literal and .expectJSONTypes() if we're checking the type of the response keys or validating against a regex.
|
Addressed comments :) |
| try { | ||
| const data = JSON.parse(buffer); | ||
| if (!('alerts' in data)) | ||
| throw new Error("Invalid data"); |
There was a problem hiding this comment.
Could you change this to be lower case please invalid data
Edit: never mind, just realised we aren't using this for output.
|
nice one - thanks |
This PR adds the lgtm.com alerts count for a particular project, using the public-facing API.
Full disclosure, I work at Semmle on lgtm.com.