[gem cdnjs appveyor clojars] refactor clojars, establish BaseJsonService #1702
Conversation
Generated by 🚫 dangerJS |
services/clojars/clojars.js
Outdated
|
|
||
| const { BaseJsonService } = require('../base'); | ||
| const { NotFound } = require('../errors'); | ||
| const { version: versionColor} = require('../../lib/color-formatters'); |
There was a problem hiding this comment.
Being picky: inconsistent spacing around versionColor ^^
There was a problem hiding this comment.
Surprised we don't have an eslint rule for this.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm not that familiar with ESLint, but I'm happy to take care of this and dig into it some more. 😉
There was a problem hiding this comment.
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.
| @@ -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' } | |||
There was a problem hiding this comment.
This particular line had a slightly different Accept header, any idea why?
There was a problem hiding this comment.
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.
services/clojars/clojars.js
Outdated
| static get url() { | ||
| return { | ||
| base: 'clojars/v', | ||
| format: '(.*)', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
👍 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.
|
This is great! Let's get it in! 😁 |
Refactored the clojars version badge. While I was doing it, I decided we really need to get the abstraction discussed here in place sooner rather than later.
Doing this makes the code for simple badges really terse. For example this reduces the gem version badge implementation down to: