You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1. I have rebased the latest version of the main branch into my feature branch and all conflicts
have been resolved.
2. I have added information about the change/addition to functionality to the CHANGELOG.md file under the
[Unreleased] heading.
3. I have documented my code using JDocs tags.
4. I have removed unnecessary commented out code, imports and System.out.println statements.
5. I have written JUnit tests for any new methods/classes and ensured that they pass.
6. I have created API tests for any new functionality exposed to the API.
7. If changes/additions are made to the ors-config.json file, I have added these to the ors config documentation
along with a short description of what it is for, and documented this in the Pull Request (below).
8. I have built graphs with my code of the Heidelberg.osm.gz file and run the api-tests with all test passing
9. I have referenced the Issue Number in the Pull Request (if the changes were from an issue).
10. For new features or changes involving building of graphs, I have tested on a larger dataset
(at least Germany), and the graphs build without problems (i.e. no out-of-memory errors).
11. For new features or changes involving the graphbuilding process (i.e. changing encoders, updating the
importer etc.), I have generated longer distance routes for the affected profiles with different options
(avoid features, max weight etc.) and compared these with the routes of the same parameters and start/end
points generated from the current live ORS.
If there are differences then the reasoning for these MUST be documented in the pull request.
12. I have written in the Pull Request information about the changes made including their intended usage
and why the change was needed.
13. For changes touching the API documentation, I have tested that the API playground renders correctly.
Information about the changes
Key functionality added: fixed matrix 'maximum_route' limit for explicit 'all' in destiantions and/or sources
Reason for change: matrix requests could circumvent the route limit as 'all' was counted as single entry during the check
Examples and reasons for differences between live ORS routes, and those generated from this pull request
passing 'all' to 'sources' or 'directions' in a request
was counted as 1 instead of the amount of locations, so
all-to-all matrix requests were treated as a 1x1 when
checking against the 'maximum_routes' setting.
Fixed by checking if sources/destinations is 'all' and then counting locations
Is it actually desired to allow passing a String value of "all" as a valid value to the sources/destination parameter? To be honest, I didn't even know this was possible. I always understood these parameters as integer arrays, which if not provided defaulted to all locations, i.e. arrays of [0, n-1] where n is the number of provided locations. I'm not sure whether mixing different types such as strings and integers within one parameter is a good idea. So maybe the actual issue here is about allowing passing "all" as a valid parameter value, which to me sounds a bit obsolete considering that the default is to take all locations anyway.
Is it actually desired to allow passing a String value of "all" as a valid value to the sources/destination parameter? To be honest, I didn't even know this was possible. I always understood these parameters as integer arrays, which if not provided defaulted to all locations, i.e. arrays of [0, n-1] where n is the number of provided locations. I'm not sure whether mixing different types such as strings and integers within one parameter is a good idea. So maybe the actual issue here is about allowing passing "all" as a valid parameter value, which to me sounds a bit obsolete considering that the default is to take all locations anyway.
Can we discuss this in a separate issue please. Currently you can circumvent the config limit. That's fixed with the PR. Being able to set all has been this way at least since 2018. The whole array is read in as strings and converted afterwards...
Of course that's not conceptually ideal, but i don't think that's relevant for this PR.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TheGreatRefrigerator
changed the title
Pull Request Checklist
have been resolved.
[Unreleased] heading.
along with a short description of what it is for, and documented this in the Pull Request (below).
(at least Germany), and the graphs build without problems (i.e. no out-of-memory errors).
importer etc.), I have generated longer distance routes for the affected profiles with different options
(avoid features, max weight etc.) and compared these with the routes of the same parameters and start/end
points generated from the current live ORS.
If there are differences then the reasoning for these MUST be documented in the pull request.
and why the change was needed.
Information about the changes
Examples and reasons for differences between live ORS routes, and those generated from this pull request
Required changes to ors config (if applicable)