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

[npmsioscore] Support npm score #6630

Merged
merged 12 commits into from Jul 2, 2021
Merged

[npmsioscore] Support npm score #6630

merged 12 commits into from Jul 2, 2021

Conversation

atian25
Copy link
Contributor

@atian25 atian25 commented Jun 13, 2021

  • How it works: https://npms.io/about

  • Routes:

    • npms-io/final-score/vue
    • npms-io/final-score/@vue/cli
    • npms-io/maintenance-score/@vue/cli
    • npms-io/popularity-score/@vue/cli
    • npms-io/quality-score/@vue/cli

close #6629

@atian25 atian25 force-pushed the master branch 5 times, most recently from 2d67832 to 845da2f Compare June 13, 2021 03:20
@atian25 atian25 changed the title [NPM Rating] Support npm rating [NPMRating] Support npm rating Jun 13, 2021
@calebcartwright calebcartwright added the service-badge Accepted and actionable changes, features, and bugs label Jun 15, 2021
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.

Thank you for the PR @atian25!

The first pass has the foundations in place, but we'll need to iterate on some of the specifics to get this to a mergable place. Details left inline below

services/npm/npm-rating.service.js Outdated Show resolved Hide resolved
services/npm/npm-rating.service.js Outdated Show resolved Hide resolved
services/npm/npm-rating.service.js Outdated Show resolved Hide resolved
services/npm/npm-rating.service.js Outdated Show resolved Hide resolved
services/npm/npm-rating.service.js Outdated Show resolved Hide resolved
services/npm/npm-rating.service.js Outdated Show resolved Hide resolved
services/npm/npm-rating.service.js Outdated Show resolved Hide resolved
services/npm/npm-rating.service.js Outdated Show resolved Hide resolved
services/npm/npm-rating.service.js Outdated Show resolved Hide resolved
services/npm/npm-rating.tester.js Outdated Show resolved Hide resolved
@calebcartwright
Copy link
Member

Closing and reopening to unstick CI

@shields-ci
Copy link

shields-ci commented Jun 15, 2021

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

Generated by 🚫 dangerJS against 79d0217

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6630 June 15, 2021 03:28 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6630 June 15, 2021 03:37 Inactive
@atian25
Copy link
Contributor Author

atian25 commented Jun 15, 2021

@calebcartwright review again?

only except one: percentage vs star (will modify it later)

@atian25 atian25 changed the title [NPMRating] Support npm rating [NPMS-IO] Support npm rating Jun 15, 2021
@calebcartwright
Copy link
Member

@calebcartwright review again?

only except one: percentage vs star (will modify it later)

Thanks for the dialog and quick updates! I'm signing out for the night but sure another maintainer or myself will pick it up in the near future

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6630 June 15, 2021 05:35 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6630 June 15, 2021 05:49 Inactive
@atian25
Copy link
Contributor Author

atian25 commented Jun 15, 2021

@calebcartwright review again?
only except one: percentage vs star (will modify it later)

Thanks for the dialog and quick updates! I'm signing out for the night but sure another maintainer or myself will pick it up in the near future

@calebcartwright now support msg_type=grade | percentage | star for user to choose

@atian25
Copy link
Contributor Author

atian25 commented Jun 19, 2021

sorry, any progress on this?

@calebcartwright
Copy link
Member

Thank you for making the update to be explicit about the service being npms.io!

However, there's some remaining unaddressed items, with the core concern being related to formatting. There are four main things that I'd like to see changed, and below the list I've included the context and rationalization for each.

  1. Let's make the nouns more explicit (e.g. final-score instead of rating, maintenance-score)
  2. Let's use standard Shields formatting for the percentage option (including color and precision)
  3. Let's drop the custom formatting options, as star and grade formatting styles do not feel applicable for these badges
  4. Let's drop the "rating" terminology

1. More Explicit Nouns

The supported npms-derived data points the proposed badges would provide are the details of the scores as determined by npms' analysis: the three detailed scores for quality, maintenance, and popularity, along with the final score which I presume is a combination of those. npms.io provides a lot of data points beyond the scores, however, and we've even had a few previous requests for other data points. By using explicit nouns we avoid any ambiguity or subjective mappings to terminology (like "rating") not used by/within npms.io itself, and it also gives us room to grow should additional badges for the service be needed

2. Percentage formatting

These scores are presented as percentages within the npms.io site, and unsurprisingly the values in the API response are decimals between 0 and 1 clearly representing that percentage (fwiw in the places where the npm.js registry site displays the subset of these npms values it does so as percentages as well).

Accordingly, this feels like a canonical case for using Shields percentage type formatting for the message. This is done by rounding to the nearest whole number and leveraging our coveragePercentage color formatter which can be imported from color-formatters. Doing this here will ensure that the percentage messages are consistent with how Shields.io does these types of badges, which is important because consistency is a core tenant of the project.

3. Drop star/grade formatting

These two formatting types do not make sense in the context of an npms analysis score percentage and they should be removed.

We support the grade-based formatting because we provide badges for upstream services, like CodeClimate, that define and determine the grade for a project within their system, and that grade is specified in their API response. Shields does not itself internally define a grade scale for the results of some upstream service, nor should we (it's not our job to to qualify their analyses). If npms.io had their own mapping of their analysis scores to a tiered grading scale then we could certainly incorporate that within the badges, but we can't make up a grading scale on their behalf.

The star formatting just doesn't make sense in this context. These npms badges reflect the numerical results of an internal, automated analysis npms.io performs by inspecting myriad attributes of a project including (but not limited to) stats/metadata of the package on the registry, build results and code coverage, issue tracker elements, and social items like stars/forks. This is inherently an analysis and not a rating. If it helps, our ratings badges and the star formatting are for services that allow users to directly rate a project/package, analogous to how you could rate an app in your app store. Technically we could use any formatting option for any badge that worked with numeric types, but we don't do this today (star formatting isn't used anywhere outside true rating badges) and we shouldn't start now IMO.

If you feel strongly about needing to be able to have a star and/or grade format on a badge for an npms score, then that would be best handled by implementing a small custom Endpoint badge after we merge this PR to provide npms support.

4. Drop rating terminology

This has largely been covered in preceding conversation, but these badges are not rating badges within the Shields.io context, so let's drop the existing usages of "rating" (badge label, params, etc.). As an example, a better badge label would be something like score as the default and with the final-score, and could be set to the detailed variants (e.g. maintenance or maintenance score) for the others.



As a final comment, remember that the `keywords` key in the example listings adds searchable terms which aid in discovery of the badges from our site. Feel free to extend this with options beyond the current `node` if you think there are any other worthwhile search terms, and that you could create a single variable to define the terms and reuse that variable in each of the example objects

@atian25
Copy link
Contributor Author

atian25 commented Jun 21, 2021

ok, got it, thanks for the long explains, will fix it later.

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6630 July 1, 2021 08:51 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6630 July 1, 2021 09:03 Inactive
@atian25 atian25 changed the title [NPMS-IO] Support npm rating [NPMS-IO] Support npm score Jul 1, 2021
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6630 July 1, 2021 09:17 Inactive
@atian25
Copy link
Contributor Author

atian25 commented Jul 1, 2021

@calebcartwright updated

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.

Thanks for the updates @atian25, this is looking good! Couple relatively minor items remaining and then it should be good to merge

services/npms-io/score.service.js Outdated Show resolved Hide resolved
services/npms-io/score.service.js Outdated Show resolved Hide resolved
services/npms-io/score.service.js Outdated Show resolved Hide resolved
atian25 and others added 3 commits July 2, 2021 11:07
Co-authored-by: Caleb Cartwright <calebcartwright@users.noreply.github.com>
Co-authored-by: Caleb Cartwright <calebcartwright@users.noreply.github.com>
Co-authored-by: Caleb Cartwright <calebcartwright@users.noreply.github.com>
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6630 July 2, 2021 03:07 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6630 July 2, 2021 03:12 Inactive
@atian25
Copy link
Contributor Author

atian25 commented Jul 2, 2021

Thanks for the updates @atian25, this is looking good! Couple relatively minor items remaining and then it should be good to merge

sure, updated

@calebcartwright
Copy link
Member

sure, updated

Thanks, that looks like everything but the file renames

@atian25
Copy link
Contributor Author

atian25 commented Jul 2, 2021

sure, updated

Thanks, that looks like everything but the file renames

ok, done.

@calebcartwright calebcartwright changed the title [NPMS-IO] Support npm score [npmsioscore] Support npm score Jul 2, 2021
@repo-ranger repo-ranger bot merged commit d2b966e into badges:master Jul 2, 2021
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.

Badge request: npm rating
4 participants