-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
default_region: add region_a to label where available #41
Conversation
Yeah, I believe this is the correct thing to do. I took a look at this yesterday only for Brazil, and I think using the existing getRegionalValue method in all labels would work well. That method has handling for region placetypes, and preference to use |
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.
LGTM
And this will not change the FRA overseas where the region replaces the country behavior so 👍 |
@Joxit I think this change will leave Is France good as-is or should we do something similar there? |
IFAIK, we don't use region abbreviations (I didn't know there were 😅) But when you are looking for a city without duplicates, the label looks like So I think France is good as-is 😅 |
Good morning 😴 This is a custom integration for our demo, so the job is not done by the API (for a better DX, this should be done by the API..) When we receive all records, we checks duplicates and we update them. If we want this in Pelias, we will need a Middleware after the label which will check duplicated labels and rename them 🤔 So this will need a work in both the API and this module where the region part can be optional for exemple? |
Okay, I ran the acceptance tests with this change to get an idea of what a bunch of labels will look like. I think it adds a bit too many abbreviations. In particular, if the label already includes a city, and there's also a region of the same name, I don't think we should show the abbreviation. For example: We might not be able to easily detect this case without adding a bunch more logic. Looks like @Joxit might be working on exactly that in bbf316a :) |
Berlin is a good edge case for my PR... 😕 Berlin is both a city AND a region.... Just like Paris... So when we add the region part, this will add nothing more... Maybe |
Yeah, I think there are several aspects to a good solution:
|
Replaced with #43 :) |
Using the
default
schema it's often difficult to distinguish localities with the same name in the same country, this makes selecting the correct result impossible:In other cases it would be nice to see some more information about which general area of the country a place is in:
This PR adds the
region_a
value where available (except for theregion
layer itself, which would be redundant).It's been this way for a very long time but I think this is an improvement, ideally I would like to see four things in the label:
I looked at using the
region
label instead ofregion_a
but it can generate some labels which sound repetitive like "Wellington, Wellington City" and "Chiang Mai, Chiang Mai".