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

[badge/dynamic/json] User defined JSON source badge #820

Merged
merged 33 commits into from
Nov 1, 2017

Conversation

RedSparr0w
Copy link
Member

@RedSparr0w RedSparr0w commented Oct 28, 2016

Usage

Allow users to specify their own API/source of json data.

JSONPath query helper

Should also help to minimize the amount of new badges requested as users could easily create a 1 off badge for themselves.

Format

Badge url: /badge/dynamic/json.svg?

?a=b params Usage Example Example Badge
uri (required) Address where json is located uri=http://path.to.json
query (required) JSONpath expression query=$.version
label Left hand side text label=ProjectX
colorB Hex color of background for right hand side colorB=10ADED
prefix Prefix for right hand text prefix=v
suffix Suffix for right hand text suffix=%20dev
logo Logo for badge logo=twitter
style Badge style style=for-the-badge

Example badges:

JSON

url:
https://img.shields.io/badge/dynamic/json.svg?uri=https://raw.githubusercontent.com/badges/shields/master/package.json&query=$.version&label=Shields.io&prefix=v&suffix=-master&colorB=10ADED&style=flat-square
badge:

Closes #548.

@RedSparr0w
Copy link
Member Author

@espadrine Any chance i could get this reviewed and hopefully merged? 😄

@paulmelnikow paulmelnikow added the service-badge Accepted and actionable changes, features, and bugs label Mar 27, 2017
@paulmelnikow
Copy link
Member

Hi, thanks for this contribution! This is a cool feature!

We don't currently have automated tests of the vendor code. I opened #927 to discuss how to go about that. A badge like this seems complex enough that it would really benefit from that.

Related: #548

@paulmelnikow
Copy link
Member

The test framework is merged. Would you be willing to add tests?

https://github.com/badges/shields/tree/master/service-tests

@Daniel15
Copy link
Member

Daniel15 commented Apr 30, 2017

to get index.js from {"version": "1.6.10","background": { "scripts": [ "index.js" ] }} the url part would be background=>scripts=>0 which is equivalent to data['background']['scripts'][0]

What about using a standard syntax such as JSONPath (for JSON) and XPath (for XML) for this, rather than inventing a new one? You can use a library like https://yarnpkg.com/en/package/jsonpath to evaluate a JSONPath query. This would also allow more complex expressions, like getting the final item in an array.

@RedSparr0w
Copy link
Member Author

@paulmelnikow
Will have a look at the test docs and see what i can do.

@Daniel15
Had a look at the links you posted, not quite sure how easy it would be to implement.
I agree it would be better to use some sort of existing standard syntax for it though.
But compared to the usage & 4 lines of code this uses not sure how in depth/complicated it should need to be.

var items = decodeURI(match[3]).split('=>');

items.forEach(function(item){
  data = data[decodeURI(item)];
});

@paulmelnikow
Copy link
Member

For JSON, another option I just learned about is jq, which is sort of like awk for json. It could yield more power, such as conditional expressions, and counting the elements in a list.

@paulmelnikow
Copy link
Member

@RedSparr0w would you have some time to address these comments? This is an awesome feature and it would be great to get it merged.

Also, if anyone would like to adopt this PR, please feel free to do so: grab the branch, add more commits addressing the comments, and open a new PR referencing this one. Will close in one month if no one has done so.

@paulmelnikow paulmelnikow added the good first issue New contributors, join in! label Sep 22, 2017
@RedSparr0w
Copy link
Member Author

Should be able to create the test for this either today/tomorrow,
Not sure how to go about implementing the JSONPath / XPath / jq options suggested above though.

@RedSparr0w
Copy link
Member Author

@paulmelnikow Added tests,
I have had to split it into 2 separate test due to the XML and JSON endpoints.
Is there anything else that needs to be addressed in this PR before it can be merged?

@RedSparr0w
Copy link
Member Author

RedSparr0w commented Sep 23, 2017

Travis seems to be failing with the following

PR: badges/shields#820
Title: [New Badge] User defined JSON/XML source badge
Services: (2 found) new, badge
Error: Unknown services: new, badge

I assume i just need to change the PR title?
Edit: yup that fixed it

@RedSparr0w RedSparr0w changed the title [New Badge] User defined JSON/XML source badge [JSON XML] User defined JSON/XML source badge Sep 23, 2017
@paulmelnikow
Copy link
Member

Thanks so much for reviving this! I will review within the next few days, and look into JSONPath/XPath/iq as soon as I can.

@paulmelnikow paulmelnikow requested review from paulmelnikow and removed request for paulmelnikow September 24, 2017 01:28
@paulmelnikow paulmelnikow self-assigned this Sep 24, 2017
Copy link
Member

@paulmelnikow paulmelnikow left a comment

Choose a reason for hiding this comment

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

Looks good!

@@ -448,6 +471,17 @@ <h2 id="like-this"> Like This? </h2>
document.location = url;
}

function generateDynamicImage() {
var url = '/badge/dynamic/' + escapeField(dynamicImageMaker.type.value);
Copy link
Member

Choose a reason for hiding this comment

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

I think you want to start with the var url = document.getElementById('imgUrlPrefix').textContent line from makeImage() above. Otherwise it will try to serve from shields.io instead of img.shields.io in production.

Copy link
Member

Choose a reason for hiding this comment

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

To test this you can run make website. The dynamic badge won't work of course, though you should be able to confirm that it's generating URLs that begin with https://img.shields.io/.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah did not think about that, i updated it to match the static badge function.

.expectJSONTypes(Joi.object().keys({
name: Joi.equal('custom badge'),
value: Joi.equal('')
}));
Copy link
Member

Choose a reason for hiding this comment

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

Since these are both literals, you can simplify this:

.expectJSON({ name: 'custom badge', value: '' })

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch, did not see that.

t.create('JSON from uri | with prefix & suffix & label')
.get('.json?uri=https://github.com/badges/shields/raw/master/package.json&query=$.version&prefix=v&suffix= dev&label=Shields')
.expectJSONTypes(Joi.object().keys({
name: Joi.equal('Shields'),
Copy link
Member

Choose a reason for hiding this comment

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

You can replace Joi.equal('Shields') with a bare literal 'Shields'.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good to know!

@RedSparr0w
Copy link
Member Author

RedSparr0w commented Oct 31, 2017

Not sure why i had to resolve the conflicts twice,
The second "Merge branch 'master' into custom_source" is empty,
But seems to be fine now.

@paulmelnikow
Copy link
Member

Looks good to me!

@RedSparr0w RedSparr0w merged commit ebcfb10 into badges:master Nov 1, 2017
@RedSparr0w
Copy link
Member Author

Thanks!
Glad to finally have this one closed & merged. 😄

@paulmelnikow
Copy link
Member

🎃

@tooomm
Copy link
Contributor

tooomm commented Jan 17, 2018

Are there any plans to expand the feature to the initial idea with json and xml support? @RedSparr0w
I would really like to use that. 😃 👍

By the way, could you please update the starting post of this pr accordingly. You still mention xml there but drop it on the way. That could be confusing if somebody finds this old (and big pr with a lot of comments!).

@RedSparr0w
Copy link
Member Author

RedSparr0w commented Jan 17, 2018

@tooomm
Good idea! Have updated the main post.
Will hopefully have some time in the next couple of weeks to look at adding xml support.

Notes for future self:
xpath
test xml

@reddog
Copy link

reddog commented Jan 30, 2018

@RedSparr0w Thanks to everyone for this very useful project.

I have a requirement for the xpath mechanism to parse a web page in order to get the button content. As such, it would be great to know if there were a way that I might be able to help with the project (time, work and the stars permitting, of course!)?

@paulmelnikow
Copy link
Member

Hey @reddog could you open a new issue to discuss?

@reddog
Copy link

reddog commented Feb 1, 2018

@paulmelnikow Opened at #1480

@tooomm
Copy link
Contributor

tooomm commented May 17, 2018

There used to be a very helpful link (actually two), to pages that let's you check and build your query. Can you please readd them to the starting post @RedSparr0w ?

I used http://www.jsonquerytool.com/ in the meantime.

@RedSparr0w
Copy link
Member Author

@tooomm i believe it may have been: http://jsonpath.com/ , Does this look right?

@tooomm
Copy link
Contributor

tooomm commented May 18, 2018

Yes, I remember that one! Thank you. 👍

@RedSparr0w
Copy link
Member Author

No worries, will update the main post to include the link.

@mwasilew
Copy link

Is there a way to make the colorB also dynamic? Right now I think it has to be passed as hex value in URL. Loading it from the JSON/XML would solve the problem but I'm not sure that is possible. Any hints?

@RedSparr0w
Copy link
Member Author

Not currently unfortunately, But #1525 will add something in line with what you are wanting 😄

@mwasilew
Copy link

@RedSparr0w thanks, I'll wait for it to be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend service to allow values from http request
6 participants