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

Use NE names when available #2088

Merged
merged 15 commits into from
Jul 11, 2022
Merged

Use NE names when available #2088

merged 15 commits into from
Jul 11, 2022

Conversation

jeffdefacto
Copy link
Contributor

Names from Natural Earth will overwrite OSM places features names where available

Copy link
Contributor

@tgrigsby-sc tgrigsby-sc left a comment

Choose a reason for hiding this comment

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

This all makes sense to me! I have some minor cosmetic suggestions but, assuming there are no issues when one of the rows is bad (e.g. a null ne lang value should be ok), ship it!

-- if it's a country, only look it up in the iso and tlc tables
IF place_tag='country' OR place_tag='unrecognized' THEN
SELECT hstore(ARRAY[
'ne_name_ar', i.name_ar,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about the choice to do array - wouldn't just a straight hstore be better?


ELSE
SELECT hstore(ARRAY[
'ne_name_ar', sp.name_ar,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is possible, but it seems preferable to have this arg list in a var somewhere and reuse it across all these queries - you'd just have to change the alias for each of the tables to the same thing. It's just visually difficult to verify that these are all the same

Copy link
Contributor

Choose a reason for hiding this comment

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

It would also mean fewer places to update when we decide to add a country

END IF;

ELSE
SELECT hstore(ARRAY[
Copy link
Contributor

Choose a reason for hiding this comment

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

there's enough visual distance from the last comment that it's probably worth saying 'not a country' etc in a comment here

IF NOT FOUND THEN
RETURN '{}'::jsonb;
END IF;
RETURN to_jsonb(coalesce(ne_name_tags, ''::hstore));
Copy link
Contributor

Choose a reason for hiding this comment

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

oh does this coalesce turn an array into a map? if so, then ignore my previous comment about arrays

for k in props.keys():
if k.startswith(name_prefix):
language = k[len(name_prefix):]
name = props.pop(k)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe ne_name ? Makes it a little clearer what's going on.

@jeffdefacto jeffdefacto merged commit 4895d35 into master Jul 11, 2022
@jeffdefacto jeffdefacto deleted the junderwood/ne_names branch July 11, 2022 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants