-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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.
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
refactor arch user repositories [aur] service #2086
Conversation
class AurVersion extends BaseAurService { | ||
static render({ version, outOfDate }) { | ||
const color = outOfDate === null ? 'blue' : 'orange' | ||
return { message: versionText(version), color: color } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've intentionally not used renderVersionBadge()
here because we're using colour to convey a different meaning here
Generated by 🚫 dangerJS |
We seem to usually break down different badges implementations into different files, shouldn't we have version, votes and license separated? |
We've got a mix. For some services we've got the base class in one file and each service in its own file, but for example APM also defines 3 badge classes in one file: shields/services/apm/apm.service.js Lines 157 to 161 in 264f5e3
I definitely wouldn't want to define a large service family like NPM or PyPI all in one file, but I reckon when we're dealing with a couple hundred lines of code its fine. I guess its a matter of taste though. |
Fair enough. I'm a bit short on time at the moment, but will aim at reviewing this more carefully later during the week. 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, I've left a question and a small observation inline. 😉
services/aur/aur.service.js
Outdated
results: Joi.alternatives( | ||
Joi.array() | ||
.min(0) | ||
.max(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you could simply use length(0)
here.
services/aur/aur.tester.js
Outdated
@@ -16,7 +15,7 @@ t.create('version (valid)') | |||
.get('/version/yaourt.json?style=_shields_test') | |||
.expectJSONTypes( | |||
Joi.object().keys({ | |||
name: 'aur', | |||
name: 'version', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm somewhat confused here. Why has the label changed to version
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. Looking over the examples we usually use the platform name as the label on version badges so I shouldn't have changed this.
cheers :) |
Refs #1358
This PR ports the Arch User Repositories integration to the new service architecture.