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 [PUB] points badge #7918

Merged
merged 5 commits into from May 8, 2022
Merged

add [PUB] points badge #7918

merged 5 commits into from May 8, 2022

Conversation

G1Joshi
Copy link
Contributor

@G1Joshi G1Joshi commented May 1, 2022

@shields-ci
Copy link

shields-ci commented May 1, 2022

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

Generated by 🚫 dangerJS against 792c998

@G1Joshi
Copy link
Contributor Author

G1Joshi commented May 1, 2022

This PR depends on #7921.
All tests will pass after merging the above-mentioned PR.

services/pub/pub-points.service.js Outdated Show resolved Hide resolved
const score = await this.fetch({ packageName })
const grantedPoints = score.grantedPoints
const maxPoints = score.maxPoints
return this.constructor.render({ grantedPoints, maxPoints, packageName })
Copy link
Member

Choose a reason for hiding this comment

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

Is maxPoints always 130? We seem to be doing a bit of a random mix of using maxPoints in some places and hard-coding 130 in others..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, the maximum number of points for the packages or plugins is 130.

Copy link
Member

Choose a reason for hiding this comment

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

I guess what I am getting at here is:

  • If we expect maxPoints can change over time, lets use maxPoints as a variable but loosen the validation. If we validate it as Joi.number().min(130).max(130).required() then if maxPoints ever changes upstream, the badges all break because the validation fails.
  • If we're saying it can only ever be 130 then its not really buying us anything for it to be a variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, so I have made max points dynamic and added nonNegativeInteger validation. So they will not break even if max points change over time.

return {
label: 'points',
message: `${grantedPoints}/${maxPoints}`,
color: 'brightgreen',
Copy link
Member

Choose a reason for hiding this comment

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

Given there is a ceiling on this one, rather than hard-coding the color we can assign a color based on some kind of scale using floorCount()

function floorCount(value, yellow, yellowgreen, green) {
if (value <= 0) {
return 'red'
} else if (value < yellow) {
return 'yellow'
} else if (value < yellowgreen) {
return 'yellowgreen'
} else if (value < green) {
return 'green'
} else {
return 'brightgreen'
}
}
like we do with test coverage, for example:
function coveragePercentage(percentage) {
return floorCount(percentage, 80, 90, 100)
}

Is there any kind of community norm around what is considered a "good" score we could lean on here? We shouldn't make them all brightgreen even if the score is like 4/130 or whatever

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, I have added the blue color for pub points as there are no such norms for points.

Copy link
Member

Choose a reason for hiding this comment

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

With this one it seems like the scale is more esoteric. Clearly bigger numbers are better, but is 20 "Documentation" points and 0 "Platform Support" points exactly the same as 10 "Documentation" points and 10 "Platform Support" points. Maybe lets leave this one as blue/informational.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made some changes and assigned colors based on the percentage of their granted points and max points. i.e.

floorCount((grantedPoints / maxPoints) * 100, 40, 60, 80)

@G1Joshi G1Joshi requested a review from chris48s May 4, 2022 17:37
@chris48s chris48s added service-badge Accepted and actionable changes, features, and bugs squash when passing labels May 8, 2022
@chris48s
Copy link
Member

chris48s commented May 8, 2022

Cheers for working on these

@repo-ranger repo-ranger bot merged commit 377a036 into badges:master May 8, 2022
@G1Joshi
Copy link
Contributor Author

G1Joshi commented May 8, 2022

Cheers for working on these

Thanks for your cooperation.

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.

add [PUB] points badge
3 participants