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

Better encapsulate logic for configuring admin lookup #52

Closed
orangejulius opened this issue Jun 10, 2016 · 2 comments
Closed

Better encapsulate logic for configuring admin lookup #52

orangejulius opened this issue Jun 10, 2016 · 2 comments

Comments

@orangejulius
Copy link
Member

orangejulius commented Jun 10, 2016

Currently, any importer that uses admin lookup has to make decisions about how exactly admin lookup is performed. For example we currently support admin-lookup via a local set of node processes, or a remote HTTP server.This can be configured in pelias.json, but checking the config settings is currently only done in the importers.

While we definitely want to support different ways of doing admin lookup, figuring out which method to use, and setting it up, should be done within this module, so that importers don't need to care about it. Right now, a bit too much work has to be done by the code calling this module. For example, most of our importers have code similar to this right now:

var through = require( 'through2' );
var logger = require( 'pelias-logger' ).get( 'openaddresses' );
var peliasAdminLookup = require( 'pelias-wof-admin-lookup' );

function createAdminLookupStream(enable) {
  if (enable) {
    logger.info( 'Setting up admin value lookup stream.' );
    var pipResolver = peliasAdminLookup.createLocalWofPipResolver();
    return peliasAdminLookup.createLookupStream(pipResolver);
  } else {
    return through.obj(function (doc, enc, next) {
      next(null, doc);
    });
  }
}

Ideally the var pipResolver line could go away, and any configuration would be passed in without actually having to know as much about the module internals.

Related: #51

orangejulius added a commit to pelias/openstreetmap that referenced this issue Jun 10, 2016
The admin lookup stream code was incorrectly checking the wrong
property, making it harder to enable admin lookup.

Related: pelias/wof-admin-lookup#52
Fixes pelias/wof-admin-lookup#51
orangejulius added a commit to pelias/openstreetmap that referenced this issue Jun 10, 2016
The admin lookup stream code was incorrectly checking the wrong
property, making it harder to enable admin lookup.

Thanks to @DylanFrese for helping us figure this out.

Related: pelias/wof-admin-lookup#52
Fixes pelias/wof-admin-lookup#51
@orangejulius
Copy link
Member Author

@dianashk also proposed a similar improvement to the interface in #48 (comment)

@orangejulius
Copy link
Member Author

This was fixed in #89

Creating an admin lookup stream for an importer is now as simple as

const adminLookup = require('pelias-wof-admin-lookup');
adminLookup.create()

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

No branches or pull requests

1 participant