add optional popularity column to pelias csv importer

[blackmad]

To make external POIs be at the baseline scoring as OSM venues, they should have some baseline popularity, since many OSM POIs have a popularity of ~2000 which makes it hard for third party chain data to compete with it

Connects #71

[Joxit]

I don't think that we should throw errors for popularity....

[Joxit]

I don't think that we should throw errors for popularity.... This is not a required field, so a default value is enough when the data is incorrect.

[orangejulius]

I can't recall, does throw-ing here kill the whole importer or does it print a message and skip just this one row?

In general, we've had this problem across all the importers, where different people have one of two needs:

• they want the import to finish if at all possible, even if one file/record/etc fails
• they want the import to be as complete as possible, and would rather the entire import fails than some invalid data makes its way in, or some data is missed

Where needed, we've introduced flags like missingFilesAreFatal in the OA importer to toggle between the two preferences.

So, if this throw causes the whole importer to fail, then we need to provide a way to allow invalid records to be skipped. If it prints a warning/error and continues on importing other records, then it's probably fine.

[orangejulius]

Okay, looks like it does just skip that one record. Now you all know the background anyway :)

[blackmad]

How else would a user discover that they had bad data?

…

On Wed, Sep 30, 2020 at 5:59 PM Jones Magloire ***@***.***> wrote: ***@***.**** approved this pull request. I don't think that we should throw errors for popularity.... ------------------------------ In lib/streams/documentStream.js <#80 (comment)>: > @@ -5,6 +5,16 @@ const through = require( 'through2' ); const peliasModel = require( 'pelias-model' ); const NAME_REGEX = /^name(_json)?_([a-z]{2})$/i; +function getPopularity(record) { + const popularityString = getCaseInsensitive('popularity', record); + const popularity = parseInt(popularityString); + if (popularityString && (isNaN(popularity) || !/^\d+$/.test(popularityString))) { + throw new Error("Popularity must be an int, got " + popularityString) I don't think that we should throw errors for popularity.... This is not a required field, so a default value is enough when the data is incorrect. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#80 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADMZMHGJKWMZAXHAQEC24TSIOS2HANCNFSM4R6IZNOQ> .

-- *David Blackman* creative technologist & wandering help me find my purpose <http://purpose.blackmad.com>

[Joxit]

Hum... I thought it would just print a message on failure (with the try/catch). But in fact there is also a report at the end with stats.badRecordCount++; so this is OK 👌.

[orangejulius]

I added some quick docs and this now looks good to go. Thanks!