Initial Implementation #1
Conversation
markysoft
force-pushed
the
feature/initial-work
branch
from
39f6e88
to
bc11610
Compare
There was a problem hiding this comment.
There are a number of trailing commas that need adding. I've added a comment about updating to the latest version of the eslint config which should point them out. The same goes for the ordering of object keys.
There are a few modules that have been updated in the latest pharmacy-data-etl. I think I've commented on the files that applies to. It would be worth using them as the basis.
package.json
Outdated
| "coveralls": "^3.0.0", | ||
| "eslint": "^4.6.1", | ||
| "eslint-config-airbnb-base": "^12.0.0", | ||
| "eslint-config-nhsuk": "^0.8.0", |
There was a problem hiding this comment.
The latest version is 0.10.0, can it be updated please?
Also, are there other, more recent versions of other dependencies?
There was a problem hiding this comment.
All packages explicitly updated to latest
There was a problem hiding this comment.
This will need updating to the latest version of the config once the change has been merged. See nhsuk/eslint-config-nhsuk#22 for additional information
lib/config.js
Outdated
| @@ -0,0 +1,13 @@ | |||
| const version = require('../package').version; | |||
|
|
|||
| const config = { | |||
There was a problem hiding this comment.
Can the object's fields be alphabetised please?
| @@ -0,0 +1,135 @@ | |||
| const log = require('./utils/logger'); | |||
There was a problem hiding this comment.
The pharmacy-data-etl has the recent improvements in. Perhaps take that file as a basis?
There was a problem hiding this comment.
I brought the changes across, I've removed a few things though.
The lastRunDate is related to the ETL and not used within the toolkit, so that has been pushed up to the ETL file in the shis data ETL branch I'm verifying this against.
I've removed the tracking of the modifiedIDs and its display in the summary for now as this is not used in the shis ETL. This should be revisited when the pharmacy-data-etl is updated to use the toolkit.
I think better use could be made of tracking a modified ID list in the toolkit. At the moment the pharmacy-data-etl deletes modified records, then relies on the ETL refreshing missing records. The downside with this is if there's an error updating the record after deletion it won't be replaced. It's probably better to have out of date data than no data.
There was a problem hiding this comment.
Good point about errors during updates on modified records. Ofc, we know that would never happen!
Perhaps they should only be removed once the new data has been acquired. That would work when the modified list doesn't contain records that have been removed, as is the case for syndication.
| @@ -0,0 +1,80 @@ | |||
| const async = require('async'); | |||
There was a problem hiding this comment.
Same as above, have a look at the pharmacy-data-etl latest version.
| @@ -0,0 +1,30 @@ | |||
| const fs = require('fs'); | |||
There was a problem hiding this comment.
The latest file in pharmacy-data-etl has a changes in for the output directory coming from config.
Code changes to abide by new lint rules
|
Changes made, the lint rule doesn't seem to be picking up missing trailing commas though. |
|
The next latest version of |
lib/config.js
Outdated
| @@ -1,13 +1,17 @@ | |||
| const version = require('../package').version; | |||
|
|
|||
There was a problem hiding this comment.
Looks like a blank line has snuck in.
No description provided.