Add badge for NuGet download count, again #945

Daniel15 · 2017-04-18T05:24:58Z

NuGet download badges giveth, and NuGet download badges taketh away.

Now they're back, in ~~pog~~ V3 form.

I switched the NuGet badges from the SearchAutocompleteService to the SearchQueryService. The SearchQueryService returns download counts in addition to version numbers.

This works for both NuGet and MyGet:

I also verified that the version number badges still work as expected:

I tried to keep the style as close to the original code as possible, although this code could do with some modernization (like using promises rather than callbacks) at some point. I'm happy to help with that 😃

Closes #678

The v2 API returned XML even though we asked for JSON. MyGet is still not working. Part of #655.

Switched from SearchAutocompleteService to SearchQueryService as this service also provides download counts.

paulmelnikow · 2017-04-18T12:47:42Z

This looks great! I have a couple minor comments and I'll run some tests when you're done addressing them. Thanks for clearing up this long-standing issue.

Would you be interested in helping maintain shields? Modernization could be good… We've started using lexical scope, template strings, and arrow functions in new code. (You're welcome to use those here if you'd like.) Promises could improve error handling and reduce some of the callback boilerplate. It would be a big change, so it's something we need to plan and execute gradually. I try to avoid changing code solely for the sake of updating the style, though sometimes it's necessary. We're being conservative with server.js and try.html to avoid conflicts with the large backlog of PRs. My current focus is tests of the vendor badges (#927), which will make reviewing PRs much faster, help contributors code faster, and tell us when things break. Would you like to review #937?

paulmelnikow · 2017-04-18T12:26:58Z

server.js

+      try {
+        // Official NuGet server uses "totalDownloads" whereas MyGet uses
+        // "totaldownloads" (lowercase D). Ugh.
+        var downloads = nugetData.totalDownloads || nugetData.totaldownloads;


What if there are zero downloads? Would 'totalDownloads' in nugetData ? nugetData.totalDownloads : nugetData.totaldownloads be safer?

nugetData.totalDownloads || nugetData.totaldownloads || 0 is probably fine.

In theory, everything should be using totalDownloads and totaldownloads should not exist. Certainly there'll never be a feed in which both totalDownloads and totaldownloads exist, as many JSON parsers are not case-sensitive or have an option to disable case-sensitivity. I suspect the lowercase totaldownloads is simply a bug in MyGet.

paulmelnikow · 2017-04-18T12:28:11Z

try.html

+  </tr>
+  <tr><th> MyGet tenant: </th>
+  <td><img src='/dotnet.myget/dotnet-coreclr/dt/Microsoft.DotNet.CoreCLR.svg' alt=''/></td>
+  <td><code>https://img.shields.iodotnet.myget/dotnet-coreclr/dt/Microsoft.DotNet.CoreCLR.svg</code></td>


That link is missing a /.

paulmelnikow · 2017-04-18T12:30:07Z

server.js

-              if (filteredVersions.length > 0) {
-                versions = filteredVersions;
-              }
+            data = data.data && data.data[0];


Could you make this a more explicit check that it's an array, and with the expected length?

Daniel15 · 2017-04-19T04:46:36Z

Thanks for the review! The issues you mentioned should be fixed now.

Would you be interested in helping maintain shields?

Yeah, sure! I could try to help out 😃

Modernization could be good

I posted some ideas in #948 if you're interested!

. I try to avoid changing code solely for the sake of updating the style, though sometimes it's necessary. We're being conservative with server.js and try.html to avoid conflicts with the large backlog of PRs.

We could always go through the pull requests and close out all the stale ones (where the author hasn't responded). Then go through all the current ones and see what could be merged easily. I think it's fine to tell people that code is in the process of being cleaned up and they'll need to rebase, particularly if they've got a large PR :)

My current focus is tests of the vendor badges (#927), which will make reviewing PRs much faster, help contributors code faster, and tell us when things break

This is a great focus. In my team at work, we recently had a "quality week" where we spent the whole week improving unit/integration/functional test coverage of our app, and also improving type safety (we use Flow for typechecking all our newer JavaScript). Tests will be great as we'll feel more confident with the changes being made. 👍

paulmelnikow · 2017-04-19T18:49:04Z

… we recently had a "quality week" where we spent the whole week improving unit/integration/functional test coverage of our app, and also improving type safety …

Sounds like fun!

… we use Flow for typechecking all our newer JavaScript …

Total aside: it seems to me that TypeScript has gotten an edge recently. Why did FB choose Flow?

We could always go through the pull requests and close out all the stale ones (where the author hasn't responded). Then go through all the current ones and see what could be merged easily. I think it's fine to tell people that code is in the process of being cleaned up and they'll need to rebase, particularly if they've got a large PR :)

This is true. However, I tend to look at unmerged contributions as an unmined asset. There are some features in there I want to include – #409, #611, #774, #786 to name a few – and if those contributors don't respond, I'll fix them up myself. With the tests, that process should be fast and pleasant. I think it's realistic. 12 days ago there were 55 open pull requests and now there are 42, so I'm hopeful we can work through it by mid-May. I've also not wanted to merge things too fast, because I don't want to be too far ahead if things break on deploy, and also because I've only been onboard a month or so.

paulmelnikow · 2017-04-19T18:58:06Z

Just ran these badges! It's not quite clear that these numbers are download counts:

What do you think, could we make the left side "downloads"? If showing the repository is important, we could tack "downloads" on the right side, after the metric.

This is the npm dt badge:

Daniel15 · 2017-04-19T19:04:48Z

Oh, good catch! I forgot to change the label on the badge to say "downloads". I can do that tonight 😃

Total aside: it seems to me that TypeScript has gotten an edge recently. Why did FB choose Flow?

I remember initially, Flow did a better job at inferring types from untyped code than TypeScript did. These days, both Flow and TypeScript seem good - I don't know if one has any advantages over the other. My team use Flow mainly because Flow is built by Facebook, so we can work closely with the Flow team if we encounter any issues. The app I work on (Ads - Power Editor) was actually the first app to ever use Flow in production. We have one of the largest JS apps at Facebook so it was a good test bed 😃

Daniel15 · 2017-04-20T04:18:41Z

Text updated!

kzu · 2017-04-22T01:50:52Z

I can't find the examples in the Downloads section for NuGet now, time to bring them back? ;)

kzu · 2017-04-22T01:58:09Z

I'm just being eager to try this, but I still can't get this to work: https://img.shields.io/nuget/dt/Moq.svg

Daniel15 · 2017-04-22T02:04:21Z

This change hasn't been deployed yet. Sent from my phone.

…

On Apr 21, 2017 6:58 PM, "Daniel Cazzulino" ***@***.***> wrote: I'm just being eager to try this, but I still can't get this to work: https://img.shields.io/nuget/dt/Moq.svg — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#945 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFnHaboh6DGfTMK09Y0dGElApSrm9xXks5ryV6ygaJpZM4M_4pa> .

kzu · 2017-04-24T18:05:20Z

gentle nudge 👍

paulmelnikow · 2017-04-24T20:46:15Z

Not forgotten. We're waiting along with you.

paulmelnikow · 2017-04-28T05:55:35Z

This has been deployed. 🚀

Daniel15 referenced this pull request Apr 18, 2017

NuGet: use API v3

4df74ac

The v2 API returned XML even though we asked for JSON. MyGet is still not working. Part of #655.

This was referenced Apr 18, 2017

Bring back NuGet download stats #792

Closed

NuGet download count no longer works #678

Closed

Add badge for NuGet download count, again.

afbe203

Switched from SearchAutocompleteService to SearchQueryService as this service also provides download counts.

Daniel15 force-pushed the nuget-download branch from 1089f74 to afbe203 Compare April 18, 2017 05:29

paulmelnikow requested changes Apr 18, 2017

View reviewed changes

paulmelnikow added the service-badge New or updated service badge label Apr 18, 2017

paulmelnikow mentioned this pull request Apr 18, 2017

Nuget Shield displays soft-deleted version #867

Closed

Fix code review comments

4281401

paulmelnikow approved these changes Apr 19, 2017

View reviewed changes

Update text for NuGet download badges to "Downloads"

befc0d9

paulmelnikow merged commit c5bd4a4 into badges:master Apr 20, 2017

paulmelnikow added the pending-deploy label Apr 20, 2017

Daniel15 deleted the nuget-download branch April 21, 2017 18:44

paulmelnikow added this to the Next Deploy milestone Apr 26, 2017

Daniel15 mentioned this pull request May 9, 2017

How to get total download count via v3 API? NuGet/Home#2596

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add badge for NuGet download count, again #945

Add badge for NuGet download count, again #945

Daniel15 commented Apr 18, 2017

paulmelnikow commented Apr 18, 2017

paulmelnikow Apr 18, 2017

Daniel15 Apr 19, 2017

paulmelnikow Apr 18, 2017

paulmelnikow Apr 18, 2017

Daniel15 commented Apr 19, 2017

paulmelnikow commented Apr 19, 2017

paulmelnikow commented Apr 19, 2017

Daniel15 commented Apr 19, 2017

Daniel15 commented Apr 20, 2017

kzu commented Apr 22, 2017

kzu commented Apr 22, 2017

Daniel15 commented Apr 22, 2017 via email

kzu commented Apr 24, 2017

paulmelnikow commented Apr 24, 2017

paulmelnikow commented Apr 28, 2017

Add badge for NuGet download count, again #945

Add badge for NuGet download count, again #945

Conversation

Daniel15 commented Apr 18, 2017

paulmelnikow commented Apr 18, 2017

paulmelnikow Apr 18, 2017

Choose a reason for hiding this comment

Daniel15 Apr 19, 2017

Choose a reason for hiding this comment

paulmelnikow Apr 18, 2017

Choose a reason for hiding this comment

paulmelnikow Apr 18, 2017

Choose a reason for hiding this comment

Daniel15 commented Apr 19, 2017

paulmelnikow commented Apr 19, 2017

paulmelnikow commented Apr 19, 2017

Daniel15 commented Apr 19, 2017

Daniel15 commented Apr 20, 2017

kzu commented Apr 22, 2017

kzu commented Apr 22, 2017

Daniel15 commented Apr 22, 2017 via email

kzu commented Apr 24, 2017

paulmelnikow commented Apr 24, 2017

paulmelnikow commented Apr 28, 2017