-
-
Notifications
You must be signed in to change notification settings - Fork 164
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
Adding address unit attribute to api output and queries #1052
base: master
Are you sure you want to change the base?
Conversation
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.
This looks great and is pretty huge! I found one thing that needs to be changed for American style address formats, but otherwise its great.
middleware/localNamingConventions.js
Outdated
if(place.address_parts.hasOwnProperty('unit')) { | ||
unit = ' ' + place.address_parts.unit; | ||
} | ||
var standard = ( place.address_parts.number + unit + ' ' + place.address_parts.street ), |
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.
This line should be changed so that the unit is last. Also, the string concatenation approach is getting a bit messy here. It would be great if you updated the code to use Array.join. It can handle putting in appropriate whitespace pretty nicely
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.
Thanks for the clarification!
Yeah I agree on that. Will change the order and get it a bit less messy 😃
…ged concatination to be a bit cleaner using array.join
@orangejulius changes should be there now |
The pelias-text-analyzer has been deprecated and its code moved into the API. Please rebase this branch off of master. This should affect your code only slightly, so please move the text-analyzer changes to controller/libpostal.js. |
@trescube realised that I missed your request for changes. Moved it from text-analyzer to controller/libpostal.js |
To resolve pelias/pelias#618 more details found in comments to that issue