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

Merge this repo back into pelias/admin-lookup #48

Open
orangejulius opened this issue Jun 2, 2016 · 5 comments
Open

Merge this repo back into pelias/admin-lookup #48

orangejulius opened this issue Jun 2, 2016 · 5 comments
Labels

Comments

@orangejulius
Copy link
Member

orangejulius commented Jun 2, 2016

It feels rather silly to have two admin lookup repos, especially when one of them is deprecated. I propose we move all the code in this one into the pelias/admin-lookup repo, and then delete this one. We may change how we do admin lookup again in the future, so locking Who's on First into the title of the repository doesn't feel right.

@trescube
Copy link
Contributor

trescube commented Aug 1, 2016

Do it, I introduced the wof-admin-lookup project in my younger more inexperienced JS days.

@dianashk
Copy link
Contributor

dianashk commented Aug 1, 2016

I kinda like having the data source be explicitly called out in the name. It would be ideal if there was also a generic admin-lookup module above this that served as a factory and users could specify which type of admin data they wanted to use. That would allow us or someone in the community to write an osm-admin-lookup for example and then use pelias-config to swap it out without having any impact on the rest of the codebase.

@orangejulius
Copy link
Member Author

That's true, I like that as well. We do kinda have that with https://github.com/pelias/wof-pip-service/ though.

@dianashk
Copy link
Contributor

dianashk commented Aug 1, 2016

Just to clarify, because I think my previous statement may have been a bit vague... An importer would do something like this:

var admin_lookup = require('pelias-admin-lookup');
var config = { admin_lookup: { source: 'wof' } };

someStreams
  .pipe(admin_lookup.setup(config.admin_lookup)
  .pipe(.....);

and in the admin-lookup code it would just create the right one based on the config.

@orangejulius
Copy link
Member Author

orangejulius commented Aug 1, 2016

Sorry, my comment was also vague! :p

I really like the interface you suggested and was also thinking of something along those lines when reading your comment, and was in fact agreeing. Right now we have wof-pip-service, which has a lot of, but not all of, the logic for doing admin lookup with WOF. Maybe that could eventually become wof-admin-lookup, and this repo could be admin-lookup.

In fact this also looks like it would help solve #52, since right now a little bit too much setup is required by code calling this module.

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

No branches or pull requests

3 participants