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.

Sign up for GitHub

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

Jump to bottom

Remove large number formatting for non-US countries #3015

Merged
merged 4 commits into from
Apr 19, 2021
Merged

Remove large number formatting for non-US countries #3015

merged 4 commits into from
Apr 19, 2021

Conversation

dgearhart
Copy link
Member

@dgearhart dgearhart commented Apr 19, 2021 •
edited
Loading

Issue

Fixes #2612

Tasklist

  • Add tests
  • Add #fixes with the issue number that this PR addresses
  • Update the changelog

@dgearhart
Removed number splitting logic for non-US countries
0bc38ea 
Added tests to verify no number splitting for non-US countries
Updated tests to felect new logic
@dgearhart
Merge branch 'master' of https://github.com/valhalla/valhalla into re…
c7ed76b 
…move_large_number_formatting
@dgearhart dgearhart self-assigned this Apr 19, 2021
danpaz
danpaz reviewed Apr 19, 2021
View reviewed changes
test/gurka/test_instructions_obvious_maneuver.cc
@@ -135,13 +135,13 @@ TEST_F(InstructionsObviousManeuver, NotObviousBecauseSameNameIntersectingEdgeRam
// Verify start maneuver prior to the continune maneuver
gurka::assert::raw::expect_instructions_at_maneuver_index(result, maneuver_index,
"Drive east on US 322.", "",
"Drive east on US 3 22.",
"Drive east on US 322.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these tests updated because they're using netherlands admin?

Copy link
Member Author

Choose a reason for hiding this comment

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

@danpaz yes, exactly

Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 I'm wondering if this formatting should really be based on the request language and not the country. In Canada and even in Europe I think it's desired to pronounce the digits separately, e.g. the example in one of the tests below A120 seems like it'd be a regression if the instruction language is in English rather than French. @st4141 as you wrote the original issue #2612 do you have any thoughts on this?

Copy link
Member Author

Choose a reason for hiding this comment

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

@danpaz yes, I was thinking the same thing about the language in the future. However, right now I just wanted to fix the current issue.

Copy link

@st4141 st4141 Apr 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@danpaz I guess the formatting should be based on the language of the request, not on the country where the road is.

As a French, I expect to ear "route 123" (="road 1 23" in French) if I drive in the USA. Not "route....1....23"

I don't know what an US citizen is expecting if he drives in France using Valhalla request in "En-US". I guess he prefers to ear "road...1...23".

Copy link
Member Author

Choose a reason for hiding this comment

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

@danpaz also, tested the A120 phrase and it sounded appropriate.

Copy link
Member Author

@dgearhart dgearhart Apr 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@danpaz @st4141 Currently, the formatter is based on location. I will submit an issue to enhance to use language. However, this PR fixes the number formatting for outside the US since that should have only applied to the US.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@danpaz @st4141 Currently, the formatter is based on location. I will submit an issue to enhance to use language. However, this PR fixes the number formatting for outside the US since that should have only applied to the US.

👍 sounds good to tackle this in a separate issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an issue for using request language instead of country for the text formatter? Rest lgtm.

@dgearhart dgearhart merged commit fa59a8d into master Apr 19, 2021
@dgearhart dgearhart mentioned this pull request Apr 20, 2021
Open
@dgearhart dgearhart deleted the remove_large_number_formatting branch January 5, 2022 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danpaz danpaz danpaz left review comments

@st4141 st4141 st4141 left review comments

@mandeepsandhu mandeepsandhu mandeepsandhu approved these changes

@gknisely gknisely Awaiting requested review from gknisely

@kdiluca kdiluca Awaiting requested review from kdiluca

Assignees

@dgearhart dgearhart

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

French prononciation of route number over 99
4 participants
@dgearhart @mandeepsandhu @danpaz @st4141