-
Notifications
You must be signed in to change notification settings - Fork 66
Add ceremonial name handling #129
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
Conversation
Tests are failing because there are no translations for the const |
languages/translations/en.json
Outdated
@@ -41,7 +41,8 @@ | |||
"ox": "Keep left", | |||
"xox": "Keep in the middle", | |||
"oxo": "Keep left or right" | |||
} | |||
}, | |||
"ceremonialNamesToAvoid": ["freeway", "expressway", "highway", "fwy", "hwy", "expy"] |
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.
“Fwy”, “hwy”, and “expy” shouldn’t occur in OpenStreetMap data; any occurrences would require edits to OSM.
index.js
Outdated
@@ -139,8 +139,15 @@ module.exports = function(version, _options) { | |||
} | |||
name = name.replace(' (' + step.ref + ')', ''); | |||
|
|||
if (name && ref && name !== ref) { | |||
// In attempt to avoid using the ceremonial name of a way, |
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.
I’m not sure where the term “ceremonial name” comes from, but this PR appears to handle two common scenarios:
- Plain old road names assigned to freeways, like “Northeast Expressway”
- Memorial highway designations, like “Martin Luther King Junior Memorial Highway” or “CHP Officer David W. Manning Memorial Freeway”
Let’s use the term “highway name” to refer to both senarios.
languages/translations/en.json
Outdated
@@ -41,7 +41,8 @@ | |||
"ox": "Keep left", | |||
"xox": "Keep in the middle", | |||
"oxo": "Keep left or right" | |||
} | |||
}, | |||
"ceremonialNamesToAvoid": ["freeway", "expressway", "highway", "fwy", "hwy", "expy"] |
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 is a list of words, not full names.
languages/translations/en.json
Outdated
@@ -41,7 +41,8 @@ | |||
"ox": "Keep left", | |||
"xox": "Keep in the middle", | |||
"oxo": "Keep left or right" | |||
} | |||
}, | |||
"ceremonialNamesToAvoid": ["freeway", "expressway", "highway", "fwy", "hwy", "expy"] |
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.
It’s unlikely that Transifex would be able to handle an array like this well. Even if it does present the array to translators, they’ll have to provide exactly six words to avoid, no more, no less.
We could put this array in the overrides file, but that would shut Transifex entirely out of the process. Or we could join the list together into a single, comma-delimited string, similar to the approach we took in mapbox/mapbox-navigation-ios#254.
index.js
Outdated
// In attempt to avoid using the ceremonial name of a way, | ||
// check and see if the name includes strings like `highway` or `expressway`. | ||
// If this is true and there is a ref, use the ref. | ||
var nameIsCeremonialName = instructions[language][version].constants.ceremonialNamesToAvoid.some((stringToAvoid) => name.includes(stringToAvoid)); |
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.
It would be far better to look at the previous step’s classes
for motorway
than to rely on a word appearing in the road name. For one thing, “highway” is often used for surface streets, and even sometimes “expressway”. Moreover, in some countries, there simply aren’t good stop words that apply to only freeways.
index.js
Outdated
// the ref should be used instead of the name. | ||
var wayIsOnHighway = false; | ||
if (options && options.classes) { | ||
wayIsOnHighway = options.classes.some((className) => ['toll', 'highway'].indexOf(className) > -1); |
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.
A toll road isn’t necessarily a nameless road. In fact, I’d argue that toll roads in the U.S. are more identifiable by their names than by their route numbers, even though OpenStreetMap tends to put acronyms in ref
for map display purposes. Consider:
- “New Jersey Turnpike”, not “NJTP”
- “Garden State Parkway”, not “GSP”
- “Dallas North Tollway”, not “DNT”
- “President George Bush Turnpike” and “PGBT” are interchangeable
- “Toll 23”, not “First Coast Expressway”
- “I 10 Toll”, not “Katy Tollway”
For the most part, these toll roads are already tagged as motorway
s. However, Chickasaw Turnpike is tagged as a trunk
; it would be better to call it “Chickasaw Turnpike” than “OK 7 Spur”.
More broadly, we shouldn’t give so much prominence to purely alphabetic refs, which may be mere abbreviations of the full name. For example, all the New York Parkways have acronyms in ref
: it should be “Saw Mill River Parkway”, not “SMP”.
Given these examples, I think we should use the following logic, at least until we’re able to unambiguously identify routes based on route relations:
- If the classes contain
motorway
(highway
?) and the ref contains a digit, say just the ref. - If the classes contain
motorway
and the ref contains no digit, say only the name. - If the classes don’t contain
motorway
, say the name followed by the ref.
If you’re concerned that we’d be tossing out too many refs by requiring a digit, we could instead check whether the ref
is a single, all-caps word – so GSP
, not Orange
.
I think these heuristics would work well for North America. Perhaps @freenerd could comment on whether they make any sense in Europe.
I thought the plan was to use the new classes from OSRM to determine whether we should use name or ref. Did that turn out to be impossible? |
Noting, updated this to use the new |
wayName = name + ' (' + ref + ')'; | ||
} else if (name && ref && wayMotorway && (/\d/).test(ref)) { |
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.
Nit: drop the extra parentheses around the regular expression.
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 linter for this project actually requires them 😅
In cases where we detect a way name is a ceremonial name and not the actual highway name (I-280 vs junipero serra freeway), we should use the ref name.
This is done by inspecting the name and checking and seeing if it includes
/cc @freenerd @willwhite @1ec5