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

distance_influence should not get a default value if no value is specified #2716

Merged
merged 5 commits into from Jan 10, 2023
Merged

distance_influence should not get a default value if no value is specified #2716

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2091d33
distance_influence should only be sent to server when explicitly spec…
karussell Jan 6, 2023
1a32582
fix changelog
karussell Jan 6, 2023
3a443b5
changelog
karussell Jan 6, 2023
8ae8077
CustomModel.getDistanceInfluence: allow null values
karussell Jan 9, 2023
945f484
remove comment
karussell Jan 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
@@ -1,6 +1,6 @@
### 7.0 [not yet released]

- now one has to call hasDistanceInfluence() before calling getDistanceInfluence() to avoid a NPE and no default is used, see #1234
- call hasDistanceInfluence() before calling getDistanceInfluence() if it is required to know whether 0 was explicitly set or the default, see #2716
- there is a new, required 'import.osm.ignored_highways' configuration option that must be used to not increase the graph size and decrease performance for motorized-only routing compared to previous versions, #2702
- new osm_way_id encoded value, #2701
- the parameters vehicle, weighting, edge_based and turn_costs are no longer supported, use the profile parameter instead
Expand Down
4 changes: 4 additions & 0 deletions web-api/src/main/java/com/graphhopper/util/CustomModel.java
View file
Open in desktop
Expand Up @@ -120,6 +120,10 @@ public CustomModel setDistanceInfluence(Double distanceFactor) {
return this;
}

/**
* @return the distance influence of this CustomModel. 0 is returned if it wasn't set (i.e. it was null)
* @see #hasDistanceInfluence()
*/
Copy link
Member

Choose a reason for hiding this comment

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

Can we not just return null? null means it's not there, so exactly what we want?

Copy link
Member Author

Choose a reason for hiding this comment

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

I implemented it this way initially, but this would not be backward compatible (and might throw NPEs in rare situations) and as we and other users would do this "if null then 0 else value" code anyway I thought it might be a good compromise to do this in the getter. But no strong opinion.

Copy link
Member

Choose a reason for hiding this comment

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

Did you commit this somewhere? Where was there a problem? I would prefer to keep it as null in case it is missing. The special value 0 might even be needed for some cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, will revert it to this but mention it in the changelog.

public Double getDistanceInfluence() {
return hasDistanceInfluence() ? distanceInfluence : 0;
}
Expand Down