total github downloads for all releases + all tags

[sagiegurari]

Recreated the original pull request (#526) based on comments and with latest badges repo to avoid conflict issues.

[sagiegurari]

By the way the question from my original pull request remains:
I wasn't 100% sure about github/downloads consolidation, since the URL pattern is different in the current service and wasn't sure if it will catch shorter urls (with less params).
so there are now 2 services for the github/downloads pattern but they have different params amount.
The original:

camp.route(/^\/github\/downloads\/([^\/]+)\/([^\/]+)\/([^\/]+)\/([^\/]+)\.(svg|png|gif|jpg|json)$/,

The added (by me):

camp.route(/^\/github\/downloads\/([^\/]+)\/([^\/]+)\.(svg|png|gif|jpg|json)$/,

Will it work?

[sagiegurari]

any updates on this one?

[espadrine]

Sorry for the delay.

In the case of conflicts, the first definition wins.

However, I feel we can merge the two definitions into one.

camp.route(/^\/github\/downloads\/([^\/]+)\/([^\/]+)\/([^\/]+)\/([^\/]+)\.(svg|png|gif|jpg|json)$/,

That defines four slots: user, repo, the last two can either be:

• tag and asset_name
• "latest" and asset_name
• tag and "total"
• "latest" and "total"
• (now, for the new things from this PR) asset_name and undefined
• "total" and undefined

Whether the last one is undefined determines whether we select by tag or over all tags.

[sagiegurari]

I updated the code to look for tag total.
There is only one middleware now (the original) with my additional code in it to check for tag = total to activate the different logic.

[sagiegurari]

can you please check it?

[sagiegurari]

Any updates on this one?

[espadrine]

Sorry, every time I came back to it, I ended up taking a large amount of time trying to understand why it does not work. It turns out it was an unrelated issue with my setup.

Having tag and asset_name be set to total feels weird. In my last comment, I suggested having only one total. For instance, it feels like this should work: /github/downloads/atom/atom/total.svg. Could you detail the reason you stuck to total/total?

[sagiegurari]

It has to do with the current pattern used in the current code base which expects specific amount of args.
I asked at the past will the middleware get invoked if less params are provided during a request. If so, I can change, if not, either I write a new middleware which you didn't want (see original pull request), or just get that annoying doule total that I have now which I also not fond of

[espadrine]

I think we can make the last parameter optional with this regex: /^\/github\/downloads\/([^\/]+)\/([^\/]+)\/([^\/]+)(\/[^\/]+)?\.(svg|png|gif|jpg|json)$/.

[sagiegurari]

Right, why I didn't think of it :)
I'll do the changes today.

[sagiegurari]

I have modified the code.
If you can check it in your env, it would be great.
Thanks.

[espadrine]

There's a couple of things to fix, which I commented on https://github.com/sagiegurari/shields/commit/ec97d7a50694a776e25b223c34a5bfe0b9844ca1.

[sagiegurari]

Modified but not as you suggested exactly.
Didn't know the match[x] will be null (I use express mostly, never used Camp)
Anyhow, the asset can be provided, it is the tag that needs to be null.
if no tag, than I pull the info from all releases, but I can still filter based on the requested asset, meaning to pull from all releases the total downloads of a specific asset.
so i changed the url regex as well and moved the optional block one back, again no experience with cargo and not sure if thats ok.

as for the second comment about trailing slash, didn't see any issue in the url i'm building to fetch the info from the github rest api, so i'm not sure there is an issue (that part I did test, just not with cargo server).

[espadrine]

I logged the values of tag and release_path, and tag was "/latest", and so release_path was "tags//latest" (which still works, as most servers convert double slashes into single slashes), but not "latest", as the code presumably intends to do.

Either we must tag = tag.slice(1) somewhere, or we must compare tag to things that have a leading slash (ie, tag !== '/latest', and 'tags' + tag).

[sagiegurari]

added

  if (tag && (tag.indexOf('/') !== -1)) {
    tag = tag.split('/').join('');
  }

[espadrine]

Why can't we replace that if (…) {…} by a simple if (tag) { tag = tag.slice(1); }?

I was almost tempted to merge and do that change myself, but I may be missing something.

[sagiegurari]

I think the 'if' is safer to handle all scenarios. Might be wrong, but I think better to prevent the url issue and not by mistake mess up the tag.
To me it is also a bit more understandable in an already over complicated function.

[espadrine]

Thanks!

Given the regular expression, there cannot be more than one slash in tag, so it was safe to simply use tag.slice(1).

[sagiegurari]

Thanks 👍

[sagiegurari]

when is this going to be deployed and available for use?

[espadrine]

Thanks for pinging me! It is deployed now.

[sagiegurari]

👍 thanks, it works good