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

Github Releases Download counter #404

Merged
merged 5 commits into from
Apr 23, 2015
Merged

Github Releases Download counter #404

merged 5 commits into from
Apr 23, 2015

Conversation

ahmadnassri
Copy link
Contributor

possible combinations:

  • /github/downloads/:user/:repo/latest/total.svg
  • /github/downloads/:user/:repo/:release_id/total.svg
  • /github/downloads/:user/:repo/:release_id/:asset_name.svg

possible combinations:

- `/github/downloads/:user/:repo/latest/total.svg`
- `/github/downloads/:user/:repo/:release_id/total.svg`
- `/github/downloads/:user/:repo/:release_id/:asset_name.svg`
@Alexander01998
Copy link

How about a combination that counts all downloads of all releases?
Something like /github/downloads/:user/:repo/all/total.svg.

@ahmadnassri
Copy link
Contributor Author

@Alexander01998 I thought about it, but that could be heavy on resources as it would have to iterate over releases backward in history and make multiple HTTP calls to go through github pagination ...

happy to add it (real-simple) if server maintainers think the server can handle it...

@espadrine
Copy link
Member

Computation-wise, I agree with ahmadnassri. That operation would be way cleaner to add on github's side. Were they to give direct access to that information, we could display it.

@Alexander01998
Copy link

It's OK, I can make my own counter for my own repos.

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!

@espadrine
Copy link
Member

Thanks a lot for your contribution!

Ahmad Nassri added 2 commits April 9, 2015 19:33
+ grab releases by tag name instead of obscure ID
+ more examples
+ better labels
@ahmadnassri
Copy link
Contributor Author

@espadrine made some minor adjustments, mainly:

  • calling releases by the friendlier/commonly used tag name (instead of release id, which requires the author to call the API to find)
  • added more examples as per your suggestion.

I believe this is now final / ready.

cheers.

downloads += asset.download_count;
}
});
var label = tag === 'latest' ? 'latest version' : tag;
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 we should be silent if we're showing the latest version, since that feels like a good default (as in, people would expect that to be the case if they didn't see a version).

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 was following examples of existing badges ... they show "latest version", e.g. Crates.io, Gem ...

I'm in favor of silent if latest, or perhaps simply "latest"

happy to change to whatever makes more sense

Copy link
Member

Choose a reason for hiding this comment

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

I'm in favor of silent if latest.

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

@espadrine espadrine merged commit 5cdfeb5 into badges:master Apr 23, 2015
@espadrine
Copy link
Member

Thanks a lot!

@Alexander01998
Copy link

👍

@kefir500
Copy link

kefir500 commented Sep 6, 2015

@ahmadnassri

It would have to iterate over releases backward in history and make multiple HTTP calls.

Just curious: why multiple HTTP calls? This API call returns information on all releases in a single response. Or parsing this JSON is also a time-consuming operation?

@Alexander01998
Copy link

Or parsing this JSON is also a time-consuming operation?

When having thousands of repos each having hundred of releases, it can indeed be very time-consuming.

I made a counter for one of my own repos using Google Apps Script.
If you really need a counter for all downloads, you can use that: https://gist.github.com/Alexander01998/d5bda1558cfbc20a8ae3

@ahmadnassri
Copy link
Contributor Author

Just curious: why multiple HTTP calls? This API call returns information on all releases in a single response. Or parsing this JSON is also a time-consuming operation?

@kefir500 you are correct, I had the impression at the time (for whatever reason) that it was paginated (and honestly it should be, some of those releases count in hundreds) which would have meant many many HTTP calls ...

and as @Alexander01998 pointed out, even then there would be a good amount of processing needed for a big / large project with many releases, we already know the service has been struggling with downtime recently, and a big refactor is due ..

@ahmadnassri
Copy link
Contributor Author

and again, happy to add a big loop that goes through all releases and calculates the total if maintainers think its a good idea ...

@espadrine
Copy link
Member

I believe this is the subject of an ongoing PR.

@ahmadnassri
Copy link
Contributor Author

ah yes: #535

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.

4 participants