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

Fix #23708 Only check restrictions with subtypes #68

Merged
merged 4 commits into from
Jun 5, 2024

Conversation

michaelabon
Copy link
Contributor

Fixes JOSM #23708.

The PublicTransportMendRelationAction.isRestricted method checks against a hardcoded list of vehicle types:

["restriction", "restriction:bus", "restriction:trolleybus", ...]

However, it tries to pull out the subtype and fails on that first entry, since there is no subtype in "restriction".

This change will filter the list of restrictions for only those with subtypes.

The PublicTransportMendRelationAction.isRestricted method
checks against a hardcoded list of vehicle types:

["restriction", "restriction:bus", "restriction:trolleybus", ...]

However, it tries to pull out the subtype and fails on that first
entry, since there is no subtype in "restriction".
@GerdP
Copy link

GerdP commented Jun 4, 2024

I don't see a need to create a new array. A simple check for the length should be enough. Something like
if (s.length() <= 12) continue;

@floscher
Copy link
Member

floscher commented Jun 4, 2024

@michaelabon Thanks for the pull request!
@GerdP Agree, it increases the complexity unnecessarily. I'd prefer to avoid continue statements where possible as well, though.

I think an elegant solution could be something along the lines of ("restriction:" + routeValue).equals(s).

Also: Instead of assinging remove = false it should be possible to immediately return false and skipping the remaining iterations of the for loop.

Responding to feedback that there was too much unnecessary complexity
introduced by creating a `restrictionTypes` array based
off of the `restrictions` array.
@michaelabon
Copy link
Contributor Author

@GerdP @floscher Thanks for the feedback. I've reined in the changes to just a length check. I've removed the remove variable and just returned early out of the for loop.

I think an elegant solution could be something along the lines of ("restriction:" + routeValue).equals(s).

@floscher I don't know quite see how or where that would fit in the loop. Can you be a bit more explicit, please?

Comment on lines 1123 to 1128
String routeValue = relation.get("route");
for (String s : restrictions) {
if (s.length() <= 12)
continue;
String sub = s.substring(12);
if (routeValue.equals(sub) && rel.hasTag("type", s))
Copy link
Member

@floscher floscher Jun 4, 2024

Choose a reason for hiding this comment

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

@michaelabon Sure, I was a bit in a rush earlier. This would be how it could be written:

Suggested change
String routeValue = relation.get("route");
for (String s : restrictions) {
if (s.length() <= 12)
continue;
String sub = s.substring(12);
if (routeValue.equals(sub) && rel.hasTag("type", s))
final String routeValue = Optional.ofNullable(relation.get("route")).map(it -> "restriction:" + it).orElse("");
for (String s : restrictions) {
if (routeValue.equals(s) && rel.hasTag("type", s))

So previously the prefix restriction: was removed from s and then compared with routeValue.

But now we prepend the prefix restriction: to routeValue and then compare it with s.

@michaelabon
Copy link
Contributor Author

To test this, I've been using the runJosm gradle task and opening this session file: test test test.osm.zip

That session has a newly-created relation named TEST TEST TEST with a “start” way and an ”end” way for a bus route. That fulfills the first two steps in my Steps to Reproduce section in #23708.

@floscher floscher merged commit dfcf644 into JOSM:master Jun 5, 2024
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.

3 participants