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

Add [npm] badges for collaborator count and dependency version #2461

Merged
merged 7 commits into from
Dec 12, 2018

Conversation

paulmelnikow
Copy link
Member

@paulmelnikow paulmelnikow commented Dec 5, 2018

This adds a badge for collaborator count. When evaluating a library, it can be useful to know that there's not a single-contributor bottleneck for publishing. Having more than one collaborator is a sign of library maturity.

It adds another badge for dependency version of published dependencies, which solves a similar problem as the node-version badge. I will find this useful for making sure dependencies are up to date in a library.

There are lots of possible refinements here:

  • Explicit support for popular frameworks. These could automatically pick the right dependencies, show status colors (since we can easily cache the latest version of popular frameworks), and possibly provide options to say which versions are supported and which are used for testing.
  • package.json support, for applications, which I'll add next (dependency version badges for applications, from package.json #2259)

It also updates the base service to use a pattern instead of a regex, in the case where that's possible. Hopefully the other one will be possible after the fix in pillarjs/path-to-regexp#173.

@paulmelnikow paulmelnikow added the service-badge Accepted and actionable changes, features, and bugs label Dec 5, 2018
@shields-ci
Copy link

shields-ci commented Dec 5, 2018

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

Generated by 🚫 dangerJS against 6eba5dd

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2461 December 5, 2018 19:24 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2461 December 5, 2018 19:30 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2461 December 5, 2018 19:32 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2461 December 5, 2018 19:33 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2461 December 5, 2018 19:36 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2461 December 5, 2018 19:57 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2461 December 5, 2018 20:15 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2461 December 5, 2018 20:18 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2461 December 5, 2018 20:53 Inactive
@@ -33,15 +41,15 @@ module.exports = class NpmBase extends BaseJsonService {
if (withTag) {
return {
base,
format: '(?:@([^/]+))?/?([^/]*)/?([^/]*)',
// The trailing optional means this has to be a regex.
format: '(?:(@[^/]+)/)?([^/]*)/?([^/]*)',
Copy link
Member Author

Choose a reason for hiding this comment

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

Have moved the @ inside the capture group to harmonize with the pattern.

@paulmelnikow
Copy link
Member Author

Hey @calebcartwright, how would you feel about reviewing this PR? I've been writing a lot of code so there's a bit of a backlog to review. It'd be great to have this kind of help from you! (Also, no pressure!)

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2461 December 8, 2018 22:54 Inactive
@calebcartwright
Copy link
Member

@paulmelnikow happy to help, will do!

@paulmelnikow
Copy link
Member Author

paulmelnikow commented Dec 8, 2018

Awesome 😄

Added: Feel free to DM me in Discord if you have questions!

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Looks great to me. I had one thought/question about maintainers vs. collaborators that I added inline

@@ -20,6 +24,10 @@ const schema = Joi.object({
Joi.alternatives(Joi.string(), deprecatedLicenseObjectSchema)
)
),
maintainers: Joi.array()
Copy link
Member

Choose a reason for hiding this comment

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

First thought I had here is about the usage of maintainers vs. collaborators key. I've seen both used before although I'm honestly not sure what difference they have to npm. However, if we're going to use maintainers here would it make sense to use maintainers instead of collaborators for the service and path name?

Copy link
Member Author

Choose a reason for hiding this comment

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

The API uses maintainers, so in the schema we don't have a choice.

The npm website uses Collaborators:

screen shot 2018-12-08 at 6 41 07 pm

I figure they've updated their terminology but did not modify the API, which is why I went with collaborators.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha that makes sense


const collaboratorColor = colorScale([2, 3, 4, 6])

module.exports = class NpmCollaborators extends NpmBase {
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above in base service about consistency between maintainers + collaborators

calebcartwright
calebcartwright previously approved these changes Dec 8, 2018

const keywords = ['node']

const collaboratorColor = colorScale([2, 3, 4, 6])
Copy link
Member

Choose a reason for hiding this comment

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

Low bus factor is good, but I'm not totally convinced on trying to devise an absolute scale of what number of collaborators is 'good' or 'bad' and applying that to all projects regardless of the size of their codebase or community.

I think its reasonable to say only one maintainer is bad (and so is zero) for all projects, but maybe just report the number in blue for everything >1 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Part of my use case is discerning the size of the community, so it’s a little chicken and egg. I can see your point that we’re not in a position to say that 4 is too few or 6 is enough, and that the expectation of what’s good would change for widely used tools.

1 is bad. Should 2 be caution? (Maybe orange or yellow.) Keeping in mind a badge this administrative is less likely to be used on individual readmes than on organizational badgeboards or project comparison scorecards tracking a large number of projects.

Copy link
Member

Choose a reason for hiding this comment

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

1 is bad. Should 2 be caution? (Maybe orange or yellow.)

Yeah - that sounds reasonable. 👍

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2461 December 11, 2018 21:41 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2461 December 11, 2018 21:41 Inactive
@chris48s
Copy link
Member

btw, cheers for reviewing @calebcartwright

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2461 December 12, 2018 18:37 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2461 December 12, 2018 19:58 Inactive
@shields-deployment
Copy link

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

@paulmelnikow paulmelnikow deleted the new-npm-badges branch December 12, 2018 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants