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

Add resolve() for getting local paths #50

Closed
wants to merge 1 commit into from

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Jul 18, 2020

This PR adds a method resolve() which turns a URI into a local path - if possible. It's intended to support ros/sdformat_urdf#1

* \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.

Choose a reason for hiding this comment

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

What about using optional<std::string> instead of an empty string, just so the contract is a bit more clear?

Possible edge case: I pass this function "file://", and it resolves to "", which now intersects with the error / "ignore" mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about using optionalstd::string instead of an empty string, just so the contract is a bit more clear?

I don't think we can use std::optional yet because ROS Rolling still targets the same C++ version as ROS Foxy which is C++14.

Possible edge case: I pass this function "file://", and it resolves to "", which now intersects with the error / "ignore" mode.

If I understand correctly that case is an error, and maybe should raise an exception. It looks like file:// is not a valid file URI according to RFC 8089. file:// would mean what comes next is auth-path, and auth-path requires path-absolute from RFC 3986 which can't be empty; it requires a leading "/".

      file-URI       = file-scheme ":" file-hier-part

      file-scheme    = "file"

      file-hier-part = ( "//" auth-path )
                     / local-path

      auth-path      = [ file-auth ] path-absolute

and

      path-absolute = "/" [ segment-nz *( "/" segment ) ]

Also interesting is that it appears file: doesn't actually support relative paths. both file: and file:// lead to path-absolute which must start with a /.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

One question/thought inline.

Meta:

  1. Can we add a test for this new public method?
  2. Orthogonal, but since we are looking at it here, can we rename retriever.h to retriever.hpp, and deprecate #include <retriever.h>? It's OK to do this as a follow-up too, but please open an issue in that case.

{
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?

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.

@sloretz
Copy link
Contributor Author

sloretz commented Aug 6, 2020

Orthogonal, but since we are looking at it here, can we rename retriever.h to retriever.hpp, and deprecate #include <retriever.h>? It's OK to do this as a follow-up too, but please open an issue in that case.

Did in a separate PR #51

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz force-pushed the sloretz/add_resolve_resource branch from 2c90804 to 6ee383d Compare August 18, 2020 14:56
@sloretz
Copy link
Contributor Author

sloretz commented Sep 18, 2020

Addressing the PR feedback brought up a bunch of corner cases, enough that it seems like it would make sense to add a dependency on a URI parser. Since I no longer need this in ros/sdformat_urdf#1 I'm going to close it instead.

@sloretz sloretz closed this Sep 18, 2020
@sloretz sloretz deleted the sloretz/add_resolve_resource branch September 18, 2020 20:19
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.

4 participants