-
-
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 renderDateBadge helper; affects [aur BitbucketLastCommit chrome date eclipse factorio galaxytoolshed GiteaLastCommit GistLastCommit GithubCreatedAt GithubHacktoberfest GithubIssueDetail GithubLastCommit GithubReleaseDate GitlabLastCommit maven npm openvsx snapcraft SourceforgeLastCommit steam vaadin visualstudio wordpress] #10682
Conversation
also move - age - formatDate - formatRelativeDate to date.js
|
it is going to be unlikely we'll invoke either of these directly now, but lets calidate here too
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 double checked all services relevant are present and I think the PR covers all revelant usage for the refactor.
I have 2 comments in the review.
Thanks - both useful comments 👍 |
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.
LGTM, run in docker and tried a few of the services as a sanity test in addition to the review/CI.
There's a couple of motivations behind this PR.
1. Validation
Dayjs is quite flexible in the inputs it accepts, but it also allows you to construct invalid date objects. So for example
dayjs('not-a-date')
doesn't throw, but also doesn't yield a useful date object. I don't love this behaviour tbh, but dayjs was the easiest thing to migrate to following the demise of moment.js and I don't really have the appetite to replace it.The way round this is: If we aren't able to validate our input sufficiently before constructing a date object, we need to then check the date is valid before we do anything with it.
So really what we want to be doing is:
The problem is: This is a bit cumbersome, and sometimes we do it wrong. For example, there are places in the codebase where we are not checking
.isValid()
at all and there are some places where we are (incorrectly) checking.isValid
instead of.isValid()
e.g:https://github.com/badges/shields/blob/master/services/npm/npm-last-update.service.js#L84-L88
Whoops! Anyway, we're not really setting ourselves up for success here.
So I wanted to write the correct validation once in a helper function, and then use it everywhere.
2. Reduce duplication
There are lots and lots of places where we are writing some slight variant of:
in service classes. There is scope for us to write one helper function, use it in lots of places, and clean up a couple of inconsistencies.
To be honest, I've done a bit too much stuff all in one commit here, so this is probably a bit of a pain to review. Ideally I would have split out some of the "moving stuff from one file to another" work into separate commits. On this occasion, that's just how it went down. Sorry :(
Fundamentally, the new code in this PR is
parseDate
andrenderDateBadge
.age
,formatDate
andformatRelativeDate
(plus their tests) are just moving from one file to another with no significant modifications. Then everything else is just updating services to useparseDate
andrenderDateBadge
.