-
-
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
Add [Coincap] Cryptocurrency badges #8623
Conversation
Get last changes
|
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.
This is in pretty good shape. It is clear you have read the contributing docs and thanks for including tests.
|
||
t.create('rank').get('/bitcoin.json').expectBadge({ | ||
label: 'Bitcoin', | ||
message: '1', |
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.
For the tests that call a live service, we sholdn't hard-code a response in the test. Lets validate that this as Joi.number().integer().min(1).required()
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.
The .json return a string even being number, i had to validated it with regex.
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.
You can still use Joi.number()
to picture-check a string.
Try this as a minimal example:
import Joi from 'joi'
const schema = Joi.number().integer().min(1).required()
Joi.attempt('1', schema); // passes
Joi.attempt('0', schema); // fails
Same goes for the changepercent24hr and priceusd schemas - they can use Joi.number()
instead of a regex which should make things a bit neater.
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.
Nice! but to changepercent24hr and priceusd Joi.number().required()
doesn't worked.
const schema = Joi.object({
data: Joi.object({
changePercent24Hr: Joi.number().required(),
name: Joi.string().required(),
}).required(),
}).required()
|
||
t.create('change percent 24hr').get('/bitcoin.json').expectBadge({ | ||
label: 'Bitcoin', | ||
message: isPercentage, |
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.
Looking at the build for this branch it is failing with
ValidationError: message mismatch: "value" does not match any of the allowed types
because at the time the tests ran the change was negative and the validator isn't expecting a leading -
.
We'll need to adjust the validator to allow negative values.
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 have added negative possibilities on test-validators.js
ok?
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 spotted one more issue on the last check. The examples were rendering with label undefined. Merging with a fix for that
Added