added config validation #82
Conversation
|
We've had a couple discussions around the interface to wof-admin-lookup. One suggestion is to encapsulate all the setup of admin lookup into one function. That would really help since the config validation logic would live there too. |
trescube
force-pushed
the
add-config-validation
branch
from
a6b808d to
ad8258e
Compare
| "tap-dot": "^1.0.1", | ||
| "tape": "^4.2.2", | ||
| "semantic-release": "^6.3.2" | ||
| "tape": "^4.2.2" |
There was a problem hiding this comment.
tape should be a 'dev dependency', easily fixed with:
npm rm tape --save;
npm i tape@latest --save-dev
There was a problem hiding this comment.
This is bad formatting on github's part, dependencies are slotted as correct.
| var _ = require('lodash'); | ||
|
|
||
| let maxConcurrentReqs; |
There was a problem hiding this comment.
why is this module scope?
I only see one reference to it at the bottom
There is also another variable of another scope with the same name:
this.maxConcurrentReqs
is that intended?
|
|
||
| function RemotePIPResolver(url, config) { | ||
| function RemotePIPResolver(url) { | ||
| // prepend url with 'http://' if not already | ||
| this.normalizedUrl = _.startsWith(url, 'http://') ? url : 'http://' + url; |
There was a problem hiding this comment.
what happens in the case of a URL using the https:// protocol?
There was a problem hiding this comment.
This is existing code.
| @@ -3,6 +3,8 @@ | |||
| var logger = require('pelias-logger').get('wof-admin-lookup'); | |||
| var createPIPService = require('pelias-wof-pip-service').create; | |||
|
|
|||
| let datapath; | |||
There was a problem hiding this comment.
what is the advantage of using module scope here over dependency injection via function args?
| module.exports = { | ||
| createLookupStream: createLookupStream | ||
| module.exports = (options) => { | ||
| maxConcurrentReqs = _.get(options, 'maxConcurrentReqs', 1); |
There was a problem hiding this comment.
having these be global variables in this module seems a bit odd to me since you can make many different streams with different vars and the global will always reflect the last ones.
always log suspect records, it seems like a good thing to have
We should discuss this. The tradition has been to put configValidation in the main entry point so the module can't even be loaded if the configuration is invalid, that doesn't work the same way in this module since pelias config can be passed to the individual create functions. We should probably go with dependency injection so to the create function gets only what it needs instead of hosting the entire pelias config internally.