Skip to content
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 the ability to output badges as JSON. #283

Merged
merged 1 commit into from
Oct 18, 2014
Merged

Add the ability to output badges as JSON. #283

merged 1 commit into from
Oct 18, 2014

Conversation

reinink
Copy link
Contributor

@reinink reinink commented Oct 10, 2014

As discussed in issue #264, here is update to allow outputting badges as JSON. This took a little bit of "hackwork", since the entire system is designed to output images, not plain text. Nonetheless, it seems to work very well. Simply use .json a the file extension, and the data is returned as JSON. I've also added the CORS header right away as well.

Note, the "any badge" feature will not work with JSON, as that makes no sense.

@espadrine
Copy link
Member

I've updated the one test that my changes broke, and it's now passing. However, there are number of timeout errors still—is that normal?

Timeouts are caused by the launch of Google Chrome taking time.

(Since it only launches once on the server upon startup, it isn't an issue
in practice, only when performing tests.)

@@ -0,0 +1,4 @@
{
"name": "{{=it.text[0]}}",
"value": "{{=it.text[1]}}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be wrong (in which case tell me why), but I think this could cause breaking bugs.

Wouldn't JSON.stringify(it.text[0]) be a lot better? (Obviously, you'd need to remove the surrounding ".)

@espadrine
Copy link
Member

I'm really eager to see this merged!

@reinink
Copy link
Contributor Author

reinink commented Oct 11, 2014

All very good points, I'm away for the weekend, but I'll update this early next week and hopefully we can get live! Good point on the breaking change, didn't really think about that. Wasn't sure how it was being used outside of your website. thanks for reviewing and stay tuned.

@reinink
Copy link
Contributor Author

reinink commented Oct 14, 2014

Okay, I've refactored as discussed. I actually brought this branch current with the latest commit (7212d71). There should now be no backwards breaking changes, and the tests are all passing. Let me know if I've missed anything!

@@ -200,7 +200,7 @@ function cache(f) {
}
var badgeData = getBadgeData('vendor', data);
badgeData.text[1] = 'unresponsive';
badge(badgeData, makeSend('svg', ask.res, end));
badge(badgeData, makeSend(match[0].split('.').pop(), ask.res, end));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should note that I wasn't able to test this particular line of code, since I wasn't sure how to trigger an "unresponsive" request.

@espadrine espadrine merged commit e69d52a into badges:master Oct 18, 2014
@espadrine
Copy link
Member

This is amazing. It truly is. Thanks a lot for the great work you did.

I won't put it live today; I'll play with it some more to verify possible issues. One thing that pops up first is that the match[0].split('.').pop() trick that you mentioned can potentially throw (if match[0] is not a string, which I know shouldn't happen, but I've become superstitious about this stuff), but it isn't in a safe zone.

espadrine added a commit that referenced this pull request Oct 19, 2014
@reinink
Copy link
Contributor Author

reinink commented Oct 20, 2014

Thanks, very happy to contribute to an awesome project. ;)

Good call on not assuming that match[0].split('.').pop() will always work. I see you've already added a check here, good stuff. Let me know if you need anything else from me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants