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

Add resolve() for getting local paths #50

Closed
wants to merge 1 commit into from
Closed

Add resolve() for getting local paths #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions resource_retriever/include/resource_retriever/retriever.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,14 @@ class RESOURCE_RETRIEVER_PUBLIC Retriever
*/
MemoryResource get(const std::string & url);

/**
* \brief Resolve the URL to a file on disk
* \param url The url to resolve to a local path on disk
* \return The path on disk if the url resolves to a local path, else empty string
* \throws resource_retriever::Exception if anything goes wrong.
*/
std::string resolve(const std::string & url);

private:
Retriever(const Retriever & ret) = delete;

Expand Down
20 changes: 17 additions & 3 deletions resource_retriever/src/retriever.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,10 @@ size_t curlWriteFunc(void * buffer, size_t size, size_t nmemb, void * userp)
return size * nmemb;
}

MemoryResource Retriever::get(const std::string & url)
std::string Retriever::resolve(const std::string & url)
{
std::string mod_url = url;
if (url.find("package://") == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you didn't add this, but url.find("package://") seems too broad. It will match lots of things that are probably not intended, like:

/hey/this/is/a/path/that/happens/to/contain/package://foo/bar/baz

Is it always the case that we expect the package:// to be at the beginning of the string? If so, I recently found a neat trick that emulates str.startswith(); it goes like:

url.rfind("package://", 0)

It works because in this signature, rfind starts at the position specified in the argument, and does the string compare. If it doesn't find it, it walks backwards from that position until it hits 0. (In C++ 20, I believe startswith was added, but that's too new for us right now). What do you think about switching to something like that?

Copy link

Choose a reason for hiding this comment

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

I like the idea of requiring the beginning of the string to contain package://, and the rfind method seems reasonable.

std::string mod_url = url;
mod_url.erase(0, strlen("package://"));
size_t pos = mod_url.find("/");
if (pos == std::string::npos) {
Expand All @@ -113,7 +113,21 @@ MemoryResource Retriever::get(const std::string & url)
throw Exception(url, "Package [" + package + "] does not exist");
}

mod_url = "file://" + package_path + mod_url;
return package_path + mod_url;
}
else if (url.find("file://") == 0) {
return url.substr(strlen("file://"));
}

return "";
}

MemoryResource Retriever::get(const std::string & url)
{
std::string mod_url = url;
std::string local_path = resolve(url);
if (!local_path.empty()) {
mod_url = "file://" + local_path;
}

curl_easy_setopt(curl_handle_, CURLOPT_URL, mod_url.c_str());
Expand Down