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

extract: Detect location of whosonfirst data from pelias.json #235

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

orangejulius
Copy link
Member

@orangejulius orangejulius commented Feb 12, 2025

This change allows the Placeholder extract script to work in most cases without specifying the WOF_DIR environment variable.

Previously, unless you were using the particular arrangement of files and directories from pelias/docker, the default location the extract script looks for data (/data/whosonfirst-data/sqlite) was probably not correct.

I noticed this inconvenience when running Pelias locally without docker for the first time in quite a long time.

My guess/recollection is an older version of the extract script (pre-sqlite) was pure bash, and so checking pelias.json was less convenient than in the current Node.js script.

The WOF_DIR environment variable is left as an override, but my hope is with this change almost no one would have to use it.

This change allows the Placeholder extract script to work in most cases
_without_ specifying the `WOF_DIR` environment variable.

Previously, unless you were using the particular arrangement of files
and directories from pelias/docker, the default location the extract script
looks for data (`/data/whosonfirst-data/sqlite`) was probably not
correct.

I noticed this inconvenience when running Pelias locally _without_
docker for the first time in quite a long time.

My guess/recollection is an older version of the extract script
(pre-sqlite) was pure bash, and so checking `pelias.json` was less
convenient than in the current Node.js script.

The `WOF_DIR` environment variable is left as an override, but my hope
is with this change almost no one would have to use it.
@@ -9,7 +9,9 @@ const combinedStream = require('combined-stream');

const SQLITE_REGEX = /whosonfirst-data-[a-z0-9-]+\.db$/;

const WOF_DIR = process.env.WOF_DIR || '/data/whosonfirst-data/sqlite';
const WOF_DIR = process.env.WOF_DIR || // TODO: deprecate WOF_DIR env var after some time
config.whosonfirst.datapath || // Preferred method of finding WOF data is to autodetect from pelias.json
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With ES6 this can be written as config?.whosonfirst?.datapath which will avoid fatal errors if the parent path doesn't exist.

Likely not an issue in practise as we have a default value

Copy link
Member

@missinglink missinglink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, these paths are always kinda confusing, standardising them is a nice idea, the one I linked from the defaults is different again 🤷‍♂️

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

Successfully merging this pull request may close these issues.

2 participants