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

[Homebrew] Add homebrew downloads badge #6209

Merged
merged 2 commits into from
Feb 22, 2021

Conversation

kdheepak
Copy link
Contributor

@kdheepak kdheepak commented Feb 21, 2021

I've added a downloads route for homebrew formulae.

Screen Shot 2021-02-20 at 11 29 11 PM

Screen Shot 2021-02-20 at 11 29 39 PM

I wanted to point out a few things.

As an example, the formula for cake has the following information: https://formulae.brew.sh/api/formula/cake.json

  "analytics": {
    "install": {
      "30d": {
        "cake": 93
      },
      "90d": {
        "cake": 182
      },
      "365d": {
        "cake": 922
      }
    },
    "install_on_request": {
      "30d": {
        "cake": 91
      },
      "90d": {
        "cake": 180
      },
      "365d": {
        "cake": 902
      }
    },
    "build_error": {
      "30d": {
        "cake": 0
      }
    }
  },
  1. The name of the formula is in the key of the object. This is what I ended up doing:

    const schema = Joi.object({
      analytics: Joi.object({
        install: Joi.object({
          '30d': Joi.object().required(),
          '90d': Joi.object().required(),
          '365d': Joi.object().required(),
        }).required(),
      }).required(),
    }).required()

    I would have liked to do something like the following though:

    function schemaGenerator({ formula })
      return Joi.object({
        analytics: Joi.object({
          install: Joi.object({
            '30d': Joi.object({ `${formula}`: nonNegativeInteger }).required(),
            '90d': Joi.object({ `${formula}`: nonNegativeInteger }).required(),
            '365d': Joi.object({ `${formula}`: nonNegativeInteger }).required(),
          }).required(),
        }).required(),
      }).required()
    }
    
    ...
    
      async fetch({ formula }) {
        const schema = schemaGenerator({ formula }) // generate schema based on formula
        return this._requestJson({
          schema,
          url: `https://formulae.brew.sh/api/formula/${formula}.json`,
          errorMessages: { 404: 'package not found' },
        })
      }

    But I get the following error:

    [server] 0221062056 Server is starting up: http://localhost:8080/
    [server] /Users/USER/gitrepos/shields/services/homebrew/homebrew-downloads.service.js:13
    [server]        '30d': Joi.object({ `${formula}`: nonNegativeInteger }).required(),
    [server]                            ^^^
    [server]
    [server] SyntaxError: Unexpected template string
    [server]     at wrapSafe (node:internal/modules/cjs/loader:1018:16)
    [server]     at Module._compile (node:internal/modules/cjs/loader:1066:27)
    [server]     at Object.Module._extensions..js (node:internal/modules/cjs/loader:1131:10)
    [server]     at Module.load (node:internal/modules/cjs/loader:967:32)
    [server]     at Function.Module._load (node:internal/modules/cjs/loader:807:14)
    [server]     at Module.require (node:internal/modules/cjs/loader:991:19)
    [server]     at require (node:internal/modules/cjs/helpers:92:18)
    [server]     at /Users/USER/gitrepos/shields/core/base-service/loader.js:26:20
    [server]     at Array.forEach (<anonymous>)
    [server]     at loadServiceClasses (/Users/USER/gitrepos/shields/core/base-service/loader.js:25:16)
    

    As such, right now I've implemented with without validating the final depth of the data. If anyone knows how to do this let me know. I looked through the documentation for joi and javascript template strings, and I wasn't able to figure it out.

  2. In the api data, there's downloads available for the last 30 days, 90 days and last 365 days. Based on other download badges for shields.io, I saw that the string /month and /year is used as part of the message. I couldn't think of a appropriate string to add for the label for 90 days, so I left it out for now. I can update the PR based on suggestions for this.

  3. There's install and install_on_request fields in the data. I believe install_on_request is the download numbers when I user asks to install a package. The install download numbers include download analytics for when the package is downloaded as a dependency as well. I think install download numbers will always be greater than or equal to install_on_request download numbers. We can make it a drop down option if required.

  4. I changed the version homebrew badge to have the label version instead of homebrew.

This PR will resolves #6190.

@shields-ci
Copy link

shields-ci commented Feb 21, 2021

Messages
📖 ✨ Thanks for your contribution to Shields, @kdheepak!

Generated by 🚫 dangerJS against 9028a67

@lgtm-com
Copy link

lgtm-com bot commented Feb 21, 2021

This pull request introduces 1 alert when merging 38170eb into 1adfbe4 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@kdheepak kdheepak force-pushed the add-homebrew-downloads-badge branch 3 times, most recently from 8e72627 to 4e93483 Compare February 21, 2021 07:58
@chris48s
Copy link
Member

Thanks for having a look at this. I've not read the code, but here's some answers to your questions..

  1. You can use square brackets instead of a template string, which will work:
function getSchema({ formula }) {
  return Joi.object({
    analytics: Joi.object({
      install: Joi.object({
        '30d': Joi.object({ [formula]: nonNegativeInteger }).required(),
        '90d': Joi.object({ [formula]: nonNegativeInteger }).required(),
        '365d': Joi.object({ [formula]: nonNegativeInteger }).required(),
      }).required(),
    }).required(),
  }).required()
}

we normally name functions as verbNoun i.e: doThing() not thingDoer()

I couldn't think of a appropriate string to add for the label for 90 days

I'd probably call it downloads per quarter /dq.

  1. Installs sounds like the metric that is most comparable with our other downloads/installs badges.

We can make it a drop down option if required.

It might be nice to at least give ourselves wiggle room in the route if we want to add it later. How about we make it homebrew/installs/:interval/:formula then if there is a clear case for adding install_on_request we can add the route in a non-confusing way.

  1. Version badges should generally use the name of the package manager as the badge label:

Screenshot at 2021-02-21 16-55-45

Can you leave this as "homebrew"

@kdheepak
Copy link
Contributor Author

I'd probably call it downloads per quarter /dq.

Homebrew doesn't seem to support having quarter in the message:

Screen Shot 2021-02-21 at 11 39 53 AM

Screen Shot 2021-02-21 at 11 40 57 AM

@kdheepak kdheepak force-pushed the add-homebrew-downloads-badge branch 2 times, most recently from 41a78b2 to fc6fa3e Compare February 21, 2021 18:49
@kdheepak
Copy link
Contributor Author

Thanks for your detailed comments! I've updated the PR based on your comments:

Screen Shot 2021-02-21 at 11 56 23 AM

Screen Shot 2021-02-21 at 11 56 46 AM

  1. I changed the version service label back to homebrew.

  2. I added a validation based on what you suggested. That worked great!

  3. I changed the route to /homebrew/installs/:interval/:formula.

The only thing that I'm not able to figure out is how to get /quarter in the string of the message.

@chris48s
Copy link
Member

No worries :) The reason the test fails with quarter is because we've not used it before so the isMetricOverTimePeriod validator doesn't know about it. You can add quarter to the regex

const isMetricOverTimePeriod = withRegex(
/^([1-9][0-9]*[kMGTPEZY]?|[1-9]\.[1-9][kMGTPEZY])\/(year|month|four weeks|week|day)$/
)
and I think your tests should pass

@kdheepak
Copy link
Contributor Author

Thanks for suggestion! I made the update.

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6209 February 21, 2021 20:35 Inactive
Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

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

Nice 👍 One final comment to address and we're good to merge.

services/homebrew/homebrew-downloads.service.js Outdated Show resolved Hide resolved
@chris48s chris48s merged commit 322eafd into badges:master Feb 22, 2021
@chris48s
Copy link
Member

Great first contribution- thanks

@kdheepak kdheepak deleted the add-homebrew-downloads-badge branch February 23, 2021 02:01
@kdheepak
Copy link
Contributor Author

Thanks for all the suggestions!

@calebcartwright
Copy link
Member

Indeed, well done @kdheepak!

@kdheepak
Copy link
Contributor Author

This is a very well organized piece of software and valuable resource to the community, thank you for all the time and effort you've put into it.

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.

Add homebrew download stats
5 participants