Refactor [Wordpress] services #3324
Conversation
- Prefer inline transform in `handle()` over `render()` - Avoid inversion of control by removing `BaseWordpress#handle()` and passing `extensionType` into `fetch()` - Move “not found” checks into `fetch()` - Cache wordpress versions instead of fetching on each request - Start to convert aliases to redirects (there are more of these which could be tackled in a follow-on) - Replace at least one route `format` with a `pattern` (I think?)
|
|
This pull request introduces 3 alerts when merging 6481b41 into 5bedbbd - view on LGTM.com new alerts:
Comment posted by LGTM.com |
| async function versionColorForWordpressVersion(version) { | ||
| const offeredVersions = await getOfferedVersions() | ||
|
|
||
| // What is this? |
There was a problem hiding this comment.
I think this code has now been moved twice, but probably it should be completely rewritten. This should also have some of its own tests. There is already a lot changing in this PR. I'm not sure it needs to be dealt with now.
| const scaled = (rating / 100) * 5 | ||
| return { | ||
| message: `${scaled.toFixed(1)}/5 (${metric(numRatings)})`, | ||
| color: floorCount(scaled, 2, 3, 4), |
There was a problem hiding this comment.
It looks like the legacy service (for Rating) used the fixed/rounded scaled value in the color scale. I think that would technically mean there's a handful of boundary cases with rating values really close to the scale steps that would end up colored differently.
For example if scaled has a value of 2.99 then I believe that would be yellowgreen, but in the legacy service (due to the .toFixed that same badge would have been green.
I'm not sure if that's a possibility given the underlying data, or how likely it would be, but felt it was worth mentioning
There was a problem hiding this comment.
Sure, it seems reasonable to use the rounded display value for the color.
handle()rather thanrender()BaseWordpress#handle(), passingextensionTypeintofetch(), and removing one layer of subclassingfetch()formatwith apattern(ref Deprecate routeformat, replacing withpattern#3329)Some of this is in line with our established patterns or makes it clearly easier to follow; some of it is arguably stylistic.