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

allow a host to have a path #967

Merged
merged 3 commits into from
Aug 12, 2023

Conversation

nilsnolde
Copy link
Contributor

@nilsnolde nilsnolde commented Aug 8, 2023

Issue

fixes #966

Now this can be done (note, we're robust to (non-)existing trailing slash in the path):

vroom -i docs/example_3.json -r osrm -a car:routing.openstreetmap.de/routed-car/ -p car:443 -c

Tasks

  • Update CHANGELOG.md (remove if irrelevant)
  • review

src/main.cpp Show resolved Hide resolved
Copy link
Collaborator

@jcoupey jcoupey left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, mentioned a couple edge cases in the comments.

scripts/format.sh Show resolved Hide resolved
src/structures/cl_args.cpp Outdated Show resolved Hide resolved
src/structures/cl_args.cpp Outdated Show resolved Hide resolved
@jcoupey
Copy link
Collaborator

jcoupey commented Aug 12, 2023

This part of the code is not touched really often, but I just merged a couple PR that introduced conflicts here. Sorry for the inconvenience...

@nilsnolde
Copy link
Contributor Author

Thanks for the review @jcoupey , comments are addressed and master merged.

@nilsnolde nilsnolde requested a review from jcoupey August 12, 2023 12:53
Copy link
Collaborator

@jcoupey jcoupey left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. One remaining improvement came to mind, I'll simply do this myself before formatting and merging.

index = host.find('/');
if (index != std::string::npos) {
path = host.substr(index + 1) + "/";
host = host.substr(0, index);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpicking: this is making a copy while we could simply host.erase(index).

@jcoupey jcoupey merged commit 4126460 into VROOM-Project:master Aug 12, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Path in URL won't work for HTTP router interface
2 participants