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

User defined XML source badge [dynamic-xml dynamic-json] #1494

Merged
merged 17 commits into from
Mar 6, 2018

Conversation

RedSparr0w
Copy link
Member

@RedSparr0w RedSparr0w commented Feb 12, 2018

Continuation of #820

Description:

This PR Adds XML support to the dynamic badges.
Other:
added a test for query param specified
changed uri param to url (uri still supported)

To do:

  • Add tests
  • Add support for multiple results
  • Update front end dynamic badge generator

Examples:

All examples based on this XML

?query= param badge
/addon/name
/addon/@id
//version
/addon/reviews/@num
//daily_users

Usage:

?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

@shields-ci
Copy link

shields-ci commented Feb 12, 2018

Messages
📖

✨ Thanks for your contribution to Shields, @RedSparr0w!

📖

Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS

server.js Outdated
} catch (e) {
switch (e.toString()){
case "TypeError: Cannot read property 'value' of undefined":
case "TypeError: Cannot read property 'firstChild' of undefined":
Copy link
Member

Choose a reason for hiding this comment

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

There is not test for this case (TypeError: Cannot read property 'firstChild' of undefined). Would you like to add it?

server.js Outdated
case "TypeError: Cannot read property 'firstChild' of undefined":
throw 'no result';
default:
throw e;
Copy link
Member

Choose a reason for hiding this comment

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

Are we able to test this part of the code?

server.js Outdated
@@ -7503,8 +7505,22 @@ cache({
var innerText = jsonpath.join(', ');
badgeData.text[1] = (prefix || '') + innerText + (suffix || '');
break;
case 'xml':
data = new dom().parseFromString(data);
var xpathdata = xpath.select(pathExpression, data, true);
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively we could check xpathdata value and remove try-catch block. What do you think about it?

let value;
if (xpathdata) {
  value = (prefix || '') + (pathExpression.indexOf('@') + 1 ? xpathdata.value : xpathdata.firstChild.data) + (suffix || '');
} else {
  value = 'no result';
  setBadgeColor(badgeData, 'lightgrey');
}
badgeData.text[1] = 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, a lot more simple!


const colorsB = mapValues(colorscheme, 'colorB');

const t = new ServiceTester({ id: 'badge/dynamic/xml', title: 'User Defined XML Source Data' });
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test for querying an attribute (/addon/@id)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Knew i forgot something! Glad you caught this.

@RedSparr0w RedSparr0w changed the title [badge/dynamic/xml] User defined XML source badge [badge/dynamic/xml] User defined XML source badge //test [badge/dynamic/json] Feb 14, 2018
@RedSparr0w RedSparr0w changed the title [badge/dynamic/xml] User defined XML source badge //test [badge/dynamic/json] User defined XML source badge [badge/dynamic/xml badge/dynamic/json] Feb 14, 2018
server.js Outdated

if (err != null || !res || res.statusCode !== 200)
throw 'inaccessible';
if (checkErrorResponse(badgeData, err, res, 'uri not found')) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to use checkErrorResponse() but not sure if uri not found || not found is the best message for a 404 error on this badge?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe file not found or resource not found?

@RedSparr0w RedSparr0w added the service-badge New or updated service badge label Feb 15, 2018
server.js Outdated

if (err != null || !res || res.statusCode !== 200)
throw 'inaccessible';
if (checkErrorResponse(badgeData, err, res, 'uri not found')) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe file not found or resource not found?

.get('.json?uri=https://services.addons.mozilla.org/en-US/firefox/api/1.5/addon/707078&query=//name')
.expectJSONTypes(Joi.object().keys({
name: 'custom badge',
value: Joi.string().regex(/^.+,\s.+$/)
Copy link
Member

Choose a reason for hiding this comment

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

Currently //name finds IndieGala Helper, Danial Nickford, GNU General Public License, version 3.0, Firefox, Firefox for Android. In .+,\s.+:

  • the first part .+ (greedy) matches IndieGala Helper, Danial Nickford, GNU General Public License, version 3.0, Firefox
  • the last part ,\s.+ matches , Firefox for Android

So this regular expression does not check if all elements are separated by a comma.

If we change query to query=/addon/compatible_applications/application/name we could compare result with exact value Firefox, Firefox for Android. What do you think about it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, That makes more sense.
Wasn't quite sure how to test it, so i just tried to test for the separator.

Copy link
Member Author

@RedSparr0w RedSparr0w Feb 15, 2018

Choose a reason for hiding this comment

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

Not sure why but the values seem to swap order occasionally.

I ran the test 10 times and 3 of those times it failed with the values swapped around
x 7
x 3
what about a regex of /^Firefox( for Android)?,\sFirefox( for Android)?$/

Copy link
Member

Choose a reason for hiding this comment

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

As far as I know XPath 1.0 (xpath is an XPath 1.0 implementation) uses node-sets to store results, which is an unordered collection of nodes. XPath 2.0 uses node sequences, which are an ordered collections (https://www.w3.org/TR/xpath/, https://www.w3.org/TR/xpath20/).
It would be great to use XPath 2.0 implementation, but I haven't found any good one.

@platan
Copy link
Member

platan commented Feb 24, 2018

Now it looks great! Would you like to update the dynamic badge section in documentation (current state below)?
screenshot-2018-2-24 shields io quality metadata badges for open source projects

@RedSparr0w RedSparr0w force-pushed the dynamic-xml branch 2 times, most recently from fc71cbd to 495f691 Compare February 27, 2018 22:44
@RedSparr0w RedSparr0w changed the title User defined XML source badge [badge/dynamic/xml badge/dynamic/json] User defined XML source badge [dynamic-xml dynamic-json] Feb 27, 2018
@RedSparr0w
Copy link
Member Author

Thanks, Have updated the front end dynamic badge generator,
But I think it looks a bit strange and am unsure how to make it better:
dynamic badge generator
specifically the query= part, any ideas @platan?

@tooomm
Copy link
Contributor

tooomm commented Feb 28, 2018

data type should have an explanation on which types are possible (json + xml). Maybe a mouse over hint? Or have it in the headline? Dynamic (json or xml)

json url should change it's label according to data type (or have a general name, source link?)

In the example at the bottom, <URI> should be <URL> to match "json url"
In one PR @paulmelnikow mentioned to use url over uri when possible.

@paulmelnikow
Copy link
Member

Thanks for bringing that up @tooomm. Unfortunately, the dynamic badge already supports a uri parameter because it came up before I realized how much things have been standardizing on url. Would be great to switch it over (while leaving support for the old one, for a while), though I don't think that needs to be dealt with in this PR.

@RedSparr0w Could you clarify which part of the UI you're not liking?

kept support for uri, incase of any already existing badges.
@RedSparr0w
Copy link
Member Author

data type should have an explanation on which types are possible (json + xml). Maybe a mouse over hint? Or have it in the headline? Dynamic (json or xml)

Good point, it was just using a datalist like the static badge colors input, but it does makes more sense to be a select, seeing as there is only 2 possible options.
image
image

json url should change it's label according to data type (or have a general name, source link?)

Have changed it to just be url,
Also updated from uriurl, but have kept support for uri so no badges will be broken.

Could you clarify which part of the UI you're not liking?

The query part, i feel like there must be a better way to convey how that param should be used.
image

@paulmelnikow
Copy link
Member

The query part, i feel like there must be a better way to convey how that param should be used.

Ah, I see, because the query format is different.

How about providing two examples, one with DATATYPE filled in with json, and the other filled in with xml? The different query will help reinforce the difference.

@RedSparr0w
Copy link
Member Author

That looks a lot better,
Thanks for the idea 😄
image

@platan
Copy link
Member

platan commented Mar 6, 2018

This looks great @RedSparr0w!
What do you think about changing $.data.dubd placeholder to query?
image

@RedSparr0w
Copy link
Member Author

@platan Good idea! that makes a lot more sense.

@RedSparr0w RedSparr0w merged commit be09cee into badges:master Mar 6, 2018
@ToJen
Copy link

ToJen commented Aug 2, 2018

I published a package to npm. How do I dynamically get the number of weekly/monthly downloads from there using an XML/JSON source ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants