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.

Sign up for GitHub

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

Github Releases Download counter #404

Merged
merged 5 commits into from
Apr 23, 2015
Merged

Github Releases Download counter #404

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7753098
Github Releases Download counter
Apr 3, 2015
478fc27
code review updates
Apr 9, 2015
8d8a88a
improvements
Apr 13, 2015
8dab4bb
Merge remote-tracking branch 'upstream/master'
Apr 17, 2015
5cdfeb5
silent if latest
Apr 18, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1966,6 +1966,50 @@ cache(function(data, match, sendBadge, request) {
});
}));

// GitHub release-download-count integration.
camp.route(/^\/github\/downloads\/([^\/]+)\/([^\/]+)\/([^\/]+)\/([^\/]+)\.(svg|png|gif|jpg|json)$/,
cache(function(data, match, sendBadge, request) {
var user = match[1]; // eg, qubyte/rubidium
var repo = match[2];
var id = match[3];
var asset_name = match[4].toLowerCase();
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 give an example of asset name apart from total?

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'm adding the comment inline with examples, any other suggestions for surfacing the examples of this value to visitors on the web page?

Copy link
Member

Choose a reason for hiding this comment

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

Adding an example badge with a specific asset would be great!

var format = match[5];
var apiUrl = 'https://api.github.com/repos/' + user + '/' + repo + '/releases/' + id;
// Using our OAuth App secret grants us 5000 req/hour
// instead of the standard 60 req/hour.
if (serverSecrets) {
apiUrl += '?client_id=' + serverSecrets.gh_client_id
+ '&client_secret=' + serverSecrets.gh_client_secret;
}
var badgeData = getBadgeData('downloads', data);
// A special User-Agent is required:
// http://developer.github.com/v3/#user-agent-required
request(apiUrl, { headers: githubHeaders }, function(err, res, buffer) {
if (err != null) {
badgeData.text[1] = 'inaccessible';
sendBadge(format, badgeData);
Copy link
Member

Choose a reason for hiding this comment

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

Could you return there?

}
try {
if ((+res.headers['x-ratelimit-remaining']) === 0) {
return; // Hope for the best in the cache.
}
var data = JSON.parse(buffer);
var downloads = 0;
data.assets.map(function (asset) {
Copy link
Member

Choose a reason for hiding this comment

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

I believe you should use forEach instead of map here, as you don't intend on constructing a new array from the existing one.

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 usually default to .map because its faster but can change to forEach for consistency if you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

That microbenchmark describes map and forEach as being roughly in the same ballpark (due to inlining work not being finished in V8), with forEach slightly faster. I believe you read the graph wrong.

If you'd like to make a change for performance reasons, I would suggest using a for loop with a cached length, as that is known to be the easiest to optimize for JS engines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, you are right, clearly further reading is required :)

if (asset_name === 'total' || asset_name === asset.name.toLowerCase()) {
downloads += asset.download_count
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a semicolon ; at the end please?

}
});
badgeData.text[1] = metric(downloads) + (asset_name? ' ' + asset_name: ' total');
badgeData.colorscheme = 'brightgreen';
sendBadge(format, badgeData);
} catch(e) {
badgeData.text[1] = 'none';
sendBadge(format, badgeData);
}
});
}));

// GitHub issues integration.
camp.route(/^\/github\/issues(-raw)?\/([^\/]+)\/([^\/]+)\.(svg|png|gif|jpg|json)$/,
cache(function(data, match, sendBadge, request) {
Expand Down
4 changes: 4 additions & 0 deletions try.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,10 @@ <h3 id="build"> Build </h3>
</tbody></table>
<h3 id="downloads"> Downloads </h3>
<table class='badge'><tbody>
<tr><th data-keywords='github'> Github: </th>
<td><img src='/github/downloads/atom/atom/latest/total.svg' alt=''/></td>
<td><code>https://img.shields.io/github/downloads/atom/atom/latest/total.svg</code></td>
</tr>
<tr><th data-keywords='node'> npm: </th>
<td><img src='/npm/dm/localeval.svg' alt=''/></td>
<td><code>https://img.shields.io/npm/dm/localeval.svg</code></td>
Expand Down