Skip to content
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

Configurable Geonames download URL #299

Merged
merged 3 commits into from
May 2, 2018
Merged

Conversation

tpedelose
Copy link
Contributor

Takes care of #239

The download function can now take a custom url - defined in the config file as imports.geonames.sourceURL from which to download a file. If sourceURL is undefined or an empty string the code defaults to the official Geonames dumps. The defined url can be a "template literal" format where ${countryCode} will be evaluated with the given countryCode.

Other changes (let me know if these should be broken out to other PRs or tossed entirely):

  • moved bin/download_data.js main code out to lib/task/download.js to match template of other files
  • renamed bin/download_data.js to bin/downloadData.js to match camelCase format of other files

tpedelose added 2 commits May 1, 2018 16:46
…ield in pelias.json; moved bin/download_data main code out to lib/task/download.js; renamed bin/download_data.js to bin/downloadData.js
@orangejulius
Copy link
Member

Hey @Tiranno,
Thanks for this PR. It looks great. My only suggestion would be to avoid the use of eval(). We don't really need that much flexibility here, and eval() opens up all sorts of possible issues.

Having the config variable be a string which is a URL prefix which is concatenated with the country code to generate the full URL should be enough.

@tpedelose
Copy link
Contributor Author

So you're thinking something like 'http://example.com/' + countryCode + '.zip'?

@orangejulius
Copy link
Member

Yup, that would be perfect

@orangejulius orangejulius merged commit ee45c0d into pelias:master May 2, 2018
@orangejulius
Copy link
Member

Just tested and this works perfectly. thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants