-
-
Notifications
You must be signed in to change notification settings - Fork 162
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
Conversation
5dbe830
to
e0c25d0
Compare
Do we want to remove the |
The Dockerfile no longer needs to be build on the libpostal_baseimage (which will probably go away soon)
Good call, will remove. |
That's an unfortunate side effect of having 2 libpostal configurations initialized: 1 for standard libpostal that uses |
A service instance needs a configuration and there are two configurations. You might be able to make it "smarter" in |
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? |
That might work for now but there are advantages to passing in the entire |
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 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. |
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
There was a problem hiding this 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 :)
🎉 This PR is included in version 3.33.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixed #1030
This PR:
address
intonumber
andstreet
(moved from sanitizers/_synthesize_analysis.js)req
(required to satisfy calling libpostal with values from different places)