Use ref when way is motorway #28
Conversation
| if let name = name, let ref = ref, name != ref, !isMotorWay { | ||
| wayName = modifyValueByKey != nil ? "\(modifyValueByKey!(.wayName, name)) (\(modifyValueByKey!(.wayName, ref)))" : "\(name) (\(ref))" | ||
| } else if let ref = ref, isMotorWay, let decimalRange = ref.rangeOfCharacter(from: CharacterSet.decimalDigits), !decimalRange.isEmpty { | ||
| wayName = ref; |
There was a problem hiding this comment.
This line should call modifyValueByKey just like the next else if case. In fact we could combine them if that would make things clearer.
|
|
||
| if let name = name, let ref = ref, name != ref, !isMotorWay { | ||
| wayName = modifyValueByKey != nil ? "\(modifyValueByKey!(.wayName, name)) (\(modifyValueByKey!(.wayName, ref)))" : "\(name) (\(ref))" | ||
| } else if let ref = ref, isMotorWay, let decimalRange = ref.rangeOfCharacter(from: CharacterSet.decimalDigits), !decimalRange.isEmpty { |
|
|
||
| if let name = name, let ref = ref, name != ref, !isMotorWay { | ||
| wayName = modifyValueByKey != nil ? "\(modifyValueByKey!(.wayName, name)) (\(modifyValueByKey!(.wayName, ref)))" : "\(name) (\(ref))" | ||
| } else if let ref = ref, isMotorWay, let decimalRange = ref.rangeOfCharacter(from: .decimalDigits), !decimalRange.isEmpty { |
There was a problem hiding this comment.
@1ec5 tried to do
} else if let ref = ref, isMotorWay, let decimalRange = ref.rangeOfCharacter(from: .decimalDigits), !decimalRange.isEmpty || name == nil {But I get a test failure with this logic. Am I using || correctly here?
There was a problem hiding this comment.
I think it's also to separate these two different cases.
There was a problem hiding this comment.
Sure, the if let is a good reason to leave the cases separate. If we were to combine them, we'd need to nest if statements.
There was a problem hiding this comment.
I think as is and not nesting is okay.
1ec5
left a comment
1ec5
left a comment
There was a problem hiding this comment.
A few more things to address, but this PR is nearly good to go.
|
|
||
| if let name = name, let ref = ref, name != ref { | ||
| if let roadClasses = roadClasses, roadClasses.contains(.motorway) { | ||
| isMotorWay = true |
There was a problem hiding this comment.
This can be written more succinctly:
let isMotorway = roadClasses?.contains(.motorway) ?? false| } | ||
|
|
||
| public func string(for obj: Any?, legIndex: Int?, numberOfLegs: Int?, modifyValueByKey: ((TokenType, String) -> String)?) -> String? { | ||
| public func string(for obj: Any?, legIndex: Int?, numberOfLegs: Int?, roadClasses: RoadClasses?, modifyValueByKey: ((TokenType, String) -> String)?) -> String? { |
There was a problem hiding this comment.
There's no semantic difference between an empty RoadClasses ([]) and nil, so let's make the type of the roadClasses argument required. The simpler version of this method above would pass in [].
There was a problem hiding this comment.
Would you mind adding documentation for this method? (The other one above is fine, because documentation is already provided by the superclass.)
Based off of Project-OSRM/osrm-text-instructions#129
If the road class is a motorway and the ref contains a number, we should announce the ref and not the name since it could be the ceremonial name.
/cc @1ec5 @freenerd @willwhite