-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 a render helper for downloads badges, run [amo ansible apm chromewebstore clojars conda crates docker dub eclipse gem githubdownloads] #7163
Conversation
|
function renderDownloadsBadge({ | ||
downloads, | ||
interval, | ||
version, | ||
labelOverride, | ||
colorOverride, | ||
messageSuffixOverride, | ||
}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this helper is a good example of a function that would benefit from a docstring comment explaining what each of these 6 params does, which ones are required/optional, etc as it is a bit more complex than something like renderLicenseBadge
or renderVersionBadge
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, and there's not actually any input validation in here currently. Do you think it'd be worthwhile to add some? E.g. ensure downloads is provided and numerical (or at least numerically coercible)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah its a good question that I don't have a completely solid answer to.
Just looking over renderVersionBadge
, renderLicenseBadge
, renderBuildStatusBadge
they're a bit of a mixed bag.
I reckon its mostly fine to delegate responsibility for passing sensible values to renderDownloadsBadge
to the service layer. My instinct is that at this layer of the application we probably don't want to be throwing a lot of exceptions that we need to remember to catch in the service layer but we probably do have some formatting helpers that do throw under some circumstances. Maybe we should be aiming to guard against outputting stuff like message: '12k/undefined'
though.
Dunno how much help that is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documented in 3eda04b. I couldn't quite convince myself to add any input validation and exception throwing so have opted to leave that possibility for a future day should the need arise
We've got a quite a large number of download count related badges, and as I'm sure folks can imagine (and see in the diff), we've got a good deal of duplication around rendering concerns because we don't have a rendering helper like we do for others (e.g. version badges). In my opinion there's a lot of opportunity for encapsulation and reuse, both with our existing badges and to make things simpler for any future such badges
This PR both creates the render helper for download count badges, and includes the updates to the first batch of service classes to take advantage. Once the PR review app is up I'll post a comment that includes before/after badges between the review app and what we do today as an additional step to ensure there's no changes introduced.