-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
improved handling of CSV comments #405
Conversation
@@ -33,22 +33,22 @@ | |||
"url": "https://github.com/pelias/geonames/issues" | |||
}, | |||
"engines": { | |||
"node": ">=l2.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow yeah, this is definitely a typo. It doesn't really affect much fortunately, but good catch!
@@ -6,7 +6,7 @@ | |||
"homepage": "https://pelias.io", | |||
"license": "MIT", | |||
"scripts": { | |||
"download_metadata": "mkdirp metadata && node bin/updateMetadata.js", | |||
"download_metadata": "mkdir -p metadata && node bin/updateMetadata.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is better 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Downloading the metadata works for me now.
agh woops, so the
I suspect there's just a weird bug in |
opened an issue upstream adaltas/node-csv#325 |
as mentioned in #404 (comment) there seems to be a weird bug with how the geonames metatadata files are encoding comments, (I think!)
using this custom comment handler stream we're able to work around the issue, although I'm still not clear why the
comment
option from https://csv.js.org/parse/options/ (andsed '/^#/d'
) doesn't do the same thing 🤷I've also taken the opportunity to do some simple housekeeping tasks:
csv-parse
modulebom
option forcsv-parse
as we have done in other modulesmkdirp
module introduced in Used mkdirp in download_metadata script #185, since that time we've consolidated on Docker and Windows has made progress in its terminal utilities, I hope it's no longer required.engines
definition inpackage.json
from>=l2.0.0
to>=12.0.0
, @orangejulius is this just a typo?The 'actual work' here is:
comment: ''
in the csv options to disable stripping comments within that libsplit2
and the newthrough
streams to handle this ourselves.resolves #404