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

Arrival instruction includes an optional named waypoint #235

Merged
merged 11 commits into from
Apr 11, 2018

Conversation

danpaz
Copy link
Contributor

@danpaz danpaz commented Apr 10, 2018

Issue

Updates compile to accept an optional parameter options.waypointName to use in the arrive instruction. When the new parameter is provided, compile replaces the default arrival instruction You have arrived at your {nth} destination with You have arrived at {waypoint_name}.

For example:

You have arrived at your first destination

becomes

You have arrived at Somewhere

For the localizations, I copied the default arrival instruction and modified it manually to create the new named template. In most languages this was pretty straightforward to do, but for some languages I made an effort but am not 100% confident with the result. I'll highlight these in comments below for feedback from anyone with better knowledge of those languages.

Tasklist

  • Add changelog entry
  • Review

/cc @1ec5 @lyzidiamond @bsudekum @allierowan @mcwhittemore

@danpaz danpaz requested review from 1ec5 and lyzidiamond April 10, 2018 19:46
@@ -62,49 +62,57 @@
"default": "Вы прибыли в {nth} пункт назначения",
"upcoming": "Вы прибудете в {nth} пункт назначения",
"short": "Вы прибыли",
"short-upcoming": "Вы скоро прибудете"
"short-upcoming": "Вы скоро прибудете",
"named": "Вы прибыли в {waypoint_name}"
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to change this text and below to "Вы прибыли в пункт назначения, {waypoint_name}". Otherwise currently used form requires prepositional case applying to {waypoint_name} value.

And maybe it's better to publish new strings templates in Transifex first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @yuryleb, does that mean the grammar rules applied on this line will not take care of it for us?

Copy link
Member

Choose a reason for hiding this comment

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

And maybe it's better to publish new strings templates in Transifex first?

One of the unfortunate aspects of OSRMTI currently is that adding a new string requires immediately adding something in each of the languages, whether that’s a copy of the English string or something hacked together based on preexisting translations. Transifex can only pick up new strings once the initial PR lands.

We could just insert English everywhere as a placeholder, but that would result in a temporary regression in all other languages. Regressing translation coverage isn’t necessarily a big deal, but some languages have taken months to update, even if it’s just a matter of adapting an existing string as this PR does. #84 and #155 track figuring out a better process for adding new strings.

Copy link
Member

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Thanks! A few comments about specific stopgap translations, but we can always address them in a separate PR once translators get a chance to see these new messages in Transifex.

@@ -62,49 +62,57 @@
"default": "Вы прибыли в {nth} пункт назначения",
"upcoming": "Вы прибудете в {nth} пункт назначения",
"short": "Вы прибыли",
"short-upcoming": "Вы скоро прибудете"
"short-upcoming": "Вы скоро прибудете",
"named": "Вы прибыли в {waypoint_name}"
Copy link
Member

Choose a reason for hiding this comment

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

The correct token to use here is probably either {waypoint_name:date} or {waypoint_name:accusative}, but I’d defer to @yuryleb. We can also fix this afterwards, once the strings are in Transifex.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually no, grammar rules work only with street names but waypoint_name could be any name :sad: So it's much easy just to change source strings to force nominal case for names than to extend grammar rules to handle any names.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, I didn’t really consider how the open-endedness could break the grammar rules’ assumptions. Since the client knows more about the waypoint name’s semantics, it could conceivably call grammarize() depending on whether it’s a street name or something else, but that wouldn’t happen by default. So I agree that putting the name in an appositive, as you suggested in #235 (comment), neatly sidesteps the problem. I think we’ll need to do the same thing for at least one other language anyways.

@@ -62,49 +62,57 @@
"default": "Đến nơi {nth}",
"upcoming": "Đến nơi {nth}",
"short": "Đến nơi",
"short-upcoming": "Đến nơi"
"short-upcoming": "Đến nơi",
"named": "Đến nơi {waypoint_name}"
Copy link
Member

Choose a reason for hiding this comment

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

“Đến {waypoint_name}” would be better, but I can also fix this up in Transifex after the fact.

@@ -62,49 +62,57 @@
"default": "您已经到达您的{nth}个目的地",
"upcoming": "您已经到达您的{nth}个目的地",
"short": "已到达目的地",
"short-upcoming": "您已经到达您的{nth}个目的地"
"short-upcoming": "您已经到达您的{nth}个目的地",
"named": "已到达目{waypoint_name}"
Copy link
Member

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 the 目 is necessary, but we can address this as part of #233 if necessary.

/cc @wingwuyf

@wingwuyf
Copy link

@1ec5 目 is not necessary, 目的地 is a single Chinese vocabulary. I think it should be "named": "已到达{waypoint_name}"

@1ec5
Copy link
Member

1ec5 commented Apr 11, 2018

9a6a980 updates the test scripts so that UPDATE=1 also updates any named translations. b9d776e fixes the Simplified Chinese named waypoint arrival translations, per #235 (comment).

@danpaz
Copy link
Contributor Author

danpaz commented Apr 11, 2018

Thanks everyone! I updated the Russian translations per #235 (comment). Planning to merge and cut a release with this feature shortly.

@danpaz danpaz merged commit 37339f7 into master Apr 11, 2018
@danpaz danpaz deleted the arrive-waypoint-name-instruction branch April 11, 2018 16:02
danpaz added a commit that referenced this pull request Apr 11, 2018
- Added a European Portuguese localization. [#229](#229)
- The Spanish localization now uses the informal imperative form instead of the formal imperative form, for consistency with the Castillian Spanish localization. [#230](#230)
- Added some abbreviations in German, Hebrew, Hungarian, Slovenian, and Ukrainian. [#226](#226)
- Added support for named waypoints in arrival instructions. [#235](#235)
@danpaz danpaz mentioned this pull request Apr 11, 2018
2 tasks
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.

4 participants