[gem cdnjs appveyor] refactor ruby gems service #1680
Conversation
This change to loadServiceClasses() allows us to define
services which either export a single service class e.g:
module.exports = class Cdnjs extends BaseService {
//...
}
or more than one. e.g:
module.exports = {
GemVersion,
GemDownloads,
GemOwner,
GemRank,
}
- move badge code to service classes - throw exceptions for errors - use let and const - change tests to expect 'downloads' label for error badges - general tidying
This allows (for example) GemVersion and GemDownloads both to use the example label 'Gem'
|
|
||
| downloads = metric(versionData.downloads_count); | ||
| } else { | ||
| throw new InvalidResponse('invalid', new Error('version is null')); |
There was a problem hiding this comment.
I'm passing a new Error() here because InvalidResponse expects underlyingError to exist. I suspect this is something we might end up doing a lot. Maybe it would be better if underlyingError were optional in the exception classes? Also InvalidResponse and Inaccessible expect their params in different orders. I can see that there's a case for both params being optional. Maybe it would be better to change the signatures so we can pass both of them an object with optional keys prettyMessage and underlyingError? That would make it easier for either/both to be optional and make the interface more consistent.
Generated by 🚫 dangerJS |
| @@ -87,7 +87,7 @@ t.create('version downloads (valid, specific version)') | |||
|
|
|||
| t.create('version downloads (package not found)') | |||
| .get('/dv/not-a-package/4.1.0.json') | |||
| .expectJSON({name: 'downloads@4.1.0', value: 'not found'}); | |||
| .expectJSON({name: 'downloads', value: 'not found'}); | |||
There was a problem hiding this comment.
I think the easiest thing to do where a badge may have a custom label like downloads@stable etc is to establish the convention that the error badges always use the default label instead of trying to pass a custom label when we throw an exception. This is what I've done here. Does anyone have an example of where this would not be a good idea?
There was a problem hiding this comment.
Can't think of any badges where this would be a problem.
| } | ||
| }); | ||
|
|
||
| return serviceClasses; |
There was a problem hiding this comment.
I think it is sensible that we should change loadServiceClasses() so we can define a service file gem.js which exports multiple classes, like this:
module.exports = {
GemVersion,
GemDownloads,
GemOwner,
GemRank,
}instead of forcing one class per file, but I'm open to feedback on this point.
|
Cheers. I'll merge this as it is for now and have a look at optional params on the exception classes as another issue. |
This time I've tackled a slightly more ambitious refactor. There's a few bits that would be useful to get feedback on. I will mark up the diff with comments..