Closed chris48s closed 1 week ago
Warnings | |
---|---|
:warning: | This PR modified service code for aur but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for bitbucket but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for chrome-web-store but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for date but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for eclipse-marketplace but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for factorio-mod-portal but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for galaxytoolshed but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for gitea but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for github/gist but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for github but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for gitlab but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for maven-central but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for npm but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for open-vsx but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for snapcraft but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for sourceforge but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for steam but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for vaadin-directory but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for visual-studio-marketplace but not its test code. That's okay so long as it's refactoring existing code. |
:warning: | This PR modified service code for wordpress but not its test code. That's okay so long as it's refactoring existing code. |
Messages | |
---|---|
:book: | :sparkles: Thanks for your contribution to Shields, @chris48s! |
Generated by :no_entry_sign: dangerJS against 9ce8948f029bac2fd0335059890889931a720aaa
Thanks - both useful comments :+1:
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
.