-
-
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
exclude name
for country
result labels
#633
Conversation
@@ -56,10 +56,20 @@ function getInitialLabel(record) { | |||
return []; |
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.
the comment above probably has to be updated
}; | ||
t.equal(generator(doc),'country name'); | ||
t.equal(generator(doc),'hierarchy country name'); |
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.
I'm kinda concerned that we are taking the hierarchy country, and not the name of the record itself. Not really because of the effect it has, but just because of the precedent it sets (or has already been set and we are now continuing). In any case it may not matter if we decide to get rid of geonames in the near future
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.
We could use the name
field but that would generate a label that we wouldn't be able to search for again. That is, geonames' name for China
is The People's Republic of China
. While libpostal understands it to be a country, we won't find anything since we only import the WOF name China
.
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.
I think being able to search for a record using the label is important so in the interest of preserving that we should keep it this way for now. I imagine a lot of this will change when we start looking at Alt-Names and internationalization, so we can revisit it then and figure out the "right" approach.
Despite my concerns, this is a huge improvement and it will be nice to finally knock out pelias/wof-admin-lookup#49. LGTM |
Fixes pelias/wof-admin-lookup#49