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

Chinese parser for OSM #1956

Merged
merged 11 commits into from
Jun 16, 2021
Merged

Chinese parser for OSM #1956

merged 11 commits into from
Jun 16, 2021

Conversation

peitili
Copy link
Contributor

@peitili peitili commented Jun 14, 2021

#1955

  • Update tests
  • Update docs

@peitili peitili marked this pull request as ready for review June 15, 2021 04:54
@peitili peitili changed the title Chinese paser for OSM Chinese parser for OSM Jun 15, 2021
@peitili peitili requested a review from nvkelso June 15, 2021 04:56
@peitili peitili requested review from iandees and tgrigsby-sc June 15, 2021 16:03
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.

nice - I'm assuming you'll add two more for the other two data sources in a followup. Nice work with the tests exactly laying out how your code works

@@ -612,6 +623,42 @@ def _convert_osm_l10n_name(x):
return LangResult(code=result, priority=priority)


def post_process_osm_zh(properties):
""" First check whether name:zh (Simplified) and name:zht(Traditional)
Copy link
Contributor

Choose a reason for hiding this comment

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

great comment

Copy link
Member

@nvkelso nvkelso left a comment

Choose a reason for hiding this comment

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

One nit to address to make sure intermediate processing data is not exported into tiles (it's been an issue in the past we should always guard against).

@peitili
Copy link
Contributor Author

peitili commented Jun 15, 2021

One nit to address to make sure intermediate processing data is not exported into tiles (it's been an issue in the past we should always guard against).

ok, I will remove the zh-default prop

Copy link
Member

@nvkelso nvkelso left a comment

Choose a reason for hiding this comment

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

For the docs checkmark, thanks for updating the TESTS.md, but please also update

https://github.com/tilezen/vector-datasource/blob/master/docs/layers.md#name-localization

With a note about name:zh (Simplified Chinese characters as used in China and Singapore) and name:zht for (Traditional Chinese characters used in Taiwan, Hong Kong, Macau) and how the data value backfilling works, and that we don't auto-translate to simplified <> traditional.

@peitili
Copy link
Contributor Author

peitili commented Jun 15, 2021

For the docs checkmark, thanks for updating the TESTS.md, but please also update

https://github.com/tilezen/vector-datasource/blob/master/docs/layers.md#name-localization

With a note about name:zh (Simplified Chinese characters as used in China and Singapore) and name:zht for (Traditional Chinese characters used in Taiwan, Hong Kong, Macau) and how the data value backfilling works, and that we don't auto-translate to simplified <> traditional.

@nvkelso
sure, do we also need to change CHANGELOG and VERSION stuffs or shall we wait until NatualEarth and WOF parser are done too?

@peitili
Copy link
Contributor Author

peitili commented Jun 15, 2021

For the docs checkmark, thanks for updating the TESTS.md, but please also update

https://github.com/tilezen/vector-datasource/blob/master/docs/layers.md#name-localization

With a note about name:zh (Simplified Chinese characters as used in China and Singapore) and name:zht for (Traditional Chinese characters used in Taiwan, Hong Kong, Macau) and how the data value backfilling works, and that we don't auto-translate to simplified <> traditional.

fixed

'name:zh': u'旧金山',
'name:zht': u'舊金山'})

self.assert_no_matching_feature(16, 10482, 25330, 'places', {'name:zh-default': u'旧金山/三藩市/舊金山'})
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Please format this onto multiple lines like the other assert for legibility / convention.

Copy link
Member

@nvkelso nvkelso left a comment

Choose a reason for hiding this comment

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

Great inaugural PR, thank you!

The documentation changes work for me, though please address the 3 nits comments I left (all formatting) before merging the PR.

docs/TESTS.md Show resolved Hide resolved
@peitili peitili merged commit 6f4a504 into master Jun 16, 2021
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.

5 participants