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

[JetBrains] Plugin version badge #1164

Merged
merged 10 commits into from Oct 12, 2017
Merged

[JetBrains] Plugin version badge #1164

merged 10 commits into from Oct 12, 2017

Conversation

ice1000
Copy link
Contributor

@ice1000 ice1000 commented Oct 12, 2017

It should be look like:

I don't have a node.js local environment. If test fail please do tell me.

@ice1000
Copy link
Contributor Author

ice1000 commented Oct 12, 2017

Related issue #1162

@paulmelnikow paulmelnikow added the service-badge Accepted and actionable changes, features, and bugs label Oct 12, 2017
server.js Outdated
@@ -6577,6 +6577,48 @@ cache(function(data, match, sendBadge, request) {
});
}));

// JetBrains Plugins repository version integration
camp.route(/^\/jetbrains\/plugin\/(v)\/([^\/]+)\.(svg|png|gif|jpg|json)$/,
Copy link
Member

Choose a reason for hiding this comment

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

Most of the code is identical with code from downloads badge (cause we are using the same API resource). Maybe we should have one function handling this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So how to do that? This is the first day I use JavaScript to develop 😢

Copy link
Member

Choose a reason for hiding this comment

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

We can have one camp.route with (d|v). switch statement should be put after checking if plugin is not found. Then handle d and v in this switch. You can use "Packagist integration" as a reference.

server.js Outdated
return sendBadge(format, badgeData);
}
var version = data['plugin-repository'].category[0]["idea-plugin"][0].version[0];
badgeData.text[1] = metric(downloads);
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 no downloads variable in this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

server.js Outdated

try {
switch (type) {
case 'd':
Copy link
Member

Choose a reason for hiding this comment

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

We have jetbrains/plugin/v in path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

t.create('downloads (plugin id from plugin.xml)')
.get('/plugin/d/org.intellij.scala.json')
.expectJSONTypes(Joi.object().keys({ name: 'downloads', value: isMetric }));

t.create('downloads (plugin id from plugin.xml)(v)')
Copy link
Member

Choose a reason for hiding this comment

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

Maybe version (plugin id from plugin.xml)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

module.exports = t;

t.create('downloads (number as a plugin id)')
.get('/plugin/d/7495.json')
.expectJSONTypes(Joi.object().keys({ name: 'downloads', value: isMetric }));

t.create('jetbrains plugin (number as a plugin id)(v)')
Copy link
Member

Choose a reason for hiding this comment

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

Maybe version (number as a plugin id)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

t.create('downloads (user friendly plugin id)')
.get('/plugin/d/1347-scala.json')
.expectJSONTypes(Joi.object().keys({ name: 'downloads', value: isMetric }));

t.create('downloads (user friendly plugin id)(v)')
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about version (user friendly plugin id)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh.... Wait a minute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

t.create('unknown plugin')
.get('/plugin/d/unknown-plugin.json')
.expectJSON({ name: 'downloads', value: 'not found' });

t.create('unknown plugin(v)')
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about version for unknown plugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@ice1000
Copy link
Contributor Author

ice1000 commented Oct 12, 2017

Why do the tests fail? I think the production code and test code for version is nearly the same as download, but why it fails...

@ice1000
Copy link
Contributor Author

ice1000 commented Oct 12, 2017

@platan What about the newest version?

  • fixed: download -> version
  • fixed: d -> v

@platan
Copy link
Member

platan commented Oct 12, 2017

Build results can be found here https://travis-ci.org/badges/shields/jobs/287207493.
This test failed: 1) JetBrains plugin/version missing required XML attribute(v)
Version is in a separate element, so you do not this test for missing downloads attribute.

@@ -93,6 +154,32 @@ t.create('missing required XML attribute')
})
.expectJSON({ name: 'downloads', value: 'invalid' });

t.create('missing required XML attribute(v)')
Copy link
Member

Choose a reason for hiding this comment

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

You do not need this test. Version value is in version element.

Copy link
Contributor Author

@ice1000 ice1000 Oct 12, 2017

Choose a reason for hiding this comment

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

Fixed. How is that now?

@ice1000
Copy link
Contributor Author

ice1000 commented Oct 12, 2017

CI check done. Please check and merge. Maybe you need squash and merge? 😉

server.js Outdated
cache(function(data, match, sendBadge, request) {
var pluginId = match[2];
var type = match[1];
var format = match[3];
Copy link
Member

Choose a reason for hiding this comment

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

Since you're not doing anything with type, you can remove it as a capture group in the regular expression. Just remove the parentheses around it. Then change match[2] to match[1] and change match[3] to match[2].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

server.js Outdated
}

try {
switch (type) {
Copy link
Member

Choose a reason for hiding this comment

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

Since there is only one case you do not need the switch statement at all.

Copy link
Member

Choose a reason for hiding this comment

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

@paulmelnikow maybe you didn't see my comments (they are under "show outdated" right know).
I suggested:
"Most of the code is identical with code from downloads badge (cause we are using the same API resource). Maybe we should have one function handling this?" and "We can have one camp.route with (d|v). switch statement should be put after checking if plugin is not found. Then handle d and v in this switch. You can use "Packagist integration" as a reference."

What do you think about this?

Copy link
Member

Choose a reason for hiding this comment

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

I see what you mean. Yes, I agree. Combining this into one route seems like a much better approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So should I do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind. I've done this.


const t = new ServiceTester({ id: 'jetbrains', title: 'JetBrains plugin' });
const t = new ServiceTester({ id: 'jetbrains', title: 'JetBrains plugin/version' });
Copy link
Member

Choose a reason for hiding this comment

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

Rather than keep updating this, you can just call it 'JetBrains'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -3,31 +3,53 @@
const Joi = require('joi');
const ServiceTester = require('./runner/service-tester');
const { isMetric } = require('./helpers/validators');
const { isSemver } = require('./helpers/validators');
Copy link
Member

Choose a reason for hiding this comment

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

const {
  isMetric,
  isSemver
} = require('./helpers/validators');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

t.create('version (number as a plugin id)')
.get('/plugin/v/7495.json')
.expectJSONTypes(Joi.object().keys({ name: 'jetbrains plugin', value: isSemver }));

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for all the great tests! Instead of interspersing these with the download tests, could you keep all the download tests together followed by all the version tests? They're a lot easier to follow that way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resorted.

@ice1000
Copy link
Contributor Author

ice1000 commented Oct 12, 2017

squash and merge? 😉

@paulmelnikow
Copy link
Member

Yeah, nice!!

@paulmelnikow paulmelnikow merged commit 8e180a0 into badges:master Oct 12, 2017
@paulmelnikow
Copy link
Member

Also, thanks for your reviewing @platan, much appreciated!

@ice1000
Copy link
Contributor Author

ice1000 commented Oct 13, 2017

https://img.shields.io/jetbrains/plugin/v/9630-a8translate.svg

Hmm, seems 404. @paulmelnikow How long will it take to see it work?

@paulmelnikow
Copy link
Member

paulmelnikow commented Oct 13, 2017

Until the next deploy. Shouldn't be too long, though it's entirely outside my control so I don't have a specific ETA.

You could keep an eye on this to see when it happens: https://github.com/badges/shields/commits/gh-pages

@ice1000
Copy link
Contributor Author

ice1000 commented Oct 13, 2017

Thank you! @paulmelnikow 👍

@ice1000
Copy link
Contributor Author

ice1000 commented Oct 13, 2017

@paulmelnikow Sorry for bothering but I forgot to add this badge to the front page. Can you do it for me? 😢

@paulmelnikow
Copy link
Member

Ah, shoot, sorry I missed that. Feel free to open another PR.

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.

None yet

3 participants