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

Microserviceify libpostal #1060

Merged
merged 7 commits into from
Nov 28, 2017
Merged

Microserviceify libpostal #1060

merged 7 commits into from
Nov 28, 2017

Conversation

trescube
Copy link
Contributor

Fixed #1030

This PR:

  • removes the pelias-text-analyzer dependency
  • moves conversion logic from pelias-text-analyzer to controller/libpostal.js
  • adds controller/structured_libpostal.js that calls libpostal for help splitting up address into number and street (moved from sanitizers/_synthesize_analysis.js)
  • adds libpostal service to schema
  • adds service/configurations/Libpostal.js that accepts a function that extracts a property from req (required to satisfy calling libpostal with values from different places)
  • fixed some typos in test/unit/schema.js

@trescube trescube added this to the API Improvements milestone Nov 14, 2017
@trescube trescube self-assigned this Nov 14, 2017
@ghost ghost added the in progress label Nov 14, 2017
@orangejulius
Copy link
Member

Do we want to remove the textAnalyzer param from the readme? Looks like its not around anymore

The Dockerfile no longer needs to be build on the libpostal_baseimage
(which will probably go away soon)
@trescube
Copy link
Contributor Author

Good call, will remove.

@orangejulius
Copy link
Member

I just tested this out locally and it works great. One thing I noticed is there are two log entries on startup for the libpostal service. What are the implications of that?
image

@trescube
Copy link
Contributor Author

That's an unfortunate side effect of having 2 libpostal configurations initialized: 1 for standard libpostal that uses req.clean.text and the other for structured which uses req.clean.parsed_text.address.

@trescube
Copy link
Contributor Author

A service instance needs a configuration and there are two configurations. You might be able to make it "smarter" in getParameters but that's burying business logic deep inside which would get confusing.

@orangejulius
Copy link
Member

It sounds like we don't have the right abstraction for the interface to libpostal then. Can the parameter to the libpostal service take exactly one parameter, which contains the actual value to parse, and then wherever is calling it contains the knowledge of which property on the req object to use?

@trescube
Copy link
Contributor Author

That might work for now but there are advantages to passing in the entire req object. For instance if other things are needed later. This is why the propertyExtractor is passed to the constructor.

@orangejulius
Copy link
Member

Personally having used the microservice-wrapper module heavily for the first time the other day I found that it was a really great interface except for the fact that its currently somewhat assumed that the entire req and res objects will be passed in. I think that's far too much context for that module to be given, even if it only uses a little bit of it. Even without the side effects in this PR I would love to see us change that.

If and when we need to send more context to microservice-wrapper, we can add params for the specific values we need to send in.

@orangejulius
Copy link
Member

orangejulius commented Nov 14, 2017

It also doesn't have to be now. I'm actually totally ok merging this as is :)

It works great and API startup times are so much faster!

Update Dockerfile for libpostal service
@ghost ghost assigned orangejulius Nov 15, 2017
@ghost ghost added the in progress label Nov 15, 2017
Copy link
Member

@orangejulius orangejulius left a comment

Choose a reason for hiding this comment

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

Anyways following up with yesterday the code looks great and we can debate encapsulation things to our hearts content at a later time :)

@ghost ghost removed the in progress label Nov 28, 2017
@trescube trescube deleted the microserviceify-libpostal branch November 28, 2017 15:11
@trescube trescube restored the microserviceify-libpostal branch February 5, 2018 21:46
@orangejulius orangejulius deleted the microserviceify-libpostal branch February 10, 2018 03:56
@pelias-bot
Copy link

🎉 This PR is included in version 3.33.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

microserviceify libpostal
3 participants