-
Notifications
You must be signed in to change notification settings - Fork 120
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
Add separate NE config for Chinese simplified and traditional #1964
Conversation
@@ -1,4 +1,4 @@ | |||
{% set ne_languages = ['ar', 'bn', 'de', 'en', 'es', 'fr', 'el', 'hi', 'hu', 'id', 'it', 'ja', 'ko', 'nl', 'pl', 'pt', 'ru', 'sv', 'tr', 'vi', 'zh'] %} | |||
{% set ne_languages = ['ar', 'bn', 'de', 'el', 'en', 'es', 'fa', 'fr', 'he', 'hi', 'hu', 'id', 'it', 'ja', 'ko', 'nl', 'pl', 'pt', 'ru', 'sv', 'tr', 'uk', 'ur', 'vi', 'zh', 'zht'] %} |
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.
zht
is the critical one, but the new NE data also includes explicate English (instead of mostly English name
singular), Farsi, Hebrew, Ukrainian, and Urdu.
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 means our integration test misses/ignores this path?
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 integration tests are based on "data fixtures" where you mock the output from a database query.
So yes, they are missing this path. This is why we should also have (eg add) "build release tests" to make sure tiles have the right content for a full end-to-end prod promotion.
{% set ne_languages = ['ar', 'bn', 'de', 'en', 'es', 'fr', 'el', 'hi', 'hu', 'id', 'it', 'ja', 'ko', 'nl', 'pl', 'pt', 'ru', 'sv', 'tr', 'vi', 'zh'] %} | ||
{% set ne_viewpoints = ['iso', 'ar', 'bd', 'br', 'cn', 'de', 'eg', 'es', 'fr', 'gb', 'gr', 'id', 'il', 'in', 'it', 'jp', 'ko', 'ma', 'nl', 'np', 'pk', 'pl', 'ps', 'pt', 'ru', 'sa', 'se', 'tr', 'tw', 'us', 'vn'] %} | ||
{% set ne_languages = ['ar', 'bn', 'de', 'el', 'en', 'es', 'fa', 'fr', 'he', 'hi', 'hu', 'id', 'it', 'ja', 'ko', 'nl', 'pl', 'pt', 'ru', 'sv', 'tr', 'uk', 'ur', 'vi', 'zh', 'zht'] %} | ||
{% set ne_viewpoints = ['iso', 'ar', 'bd', 'br', 'cn', 'de', 'eg', 'es', 'fr', 'gb', 'gr', 'id', 'il', 'in', 'it', 'jp', 'ko', 'ma', 'nl', 'np', 'pk', 'pl', 'ps', 'pt', 'ru', 'sa', 'se', 'tr', 'tw', 'ua', 'us', 'vn'] %} |
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.
Technically Ukrainian POV was added to the data, so adding that here as well so we don't forget it later.
The ZIP file of Natural Earth beta data we're loading does include POV, except for the populated places SHP which lost it along the way. I've restored the values and the next data delivery will include that. But not a blocker for this name work. |
Extends work done in #1961 and #1963
During a build we discovered the live data isn't parsed correctly because the Natural Earth config is ignorant of the new
zht
name property. Because we reusedzh
forzh-Hans
content that new data did flow thru correctly.The fix is to extend the list of NE language codes with what's in the new beta data.