#123 Declined
Repository
peci1
Branch
add-uri-resolvers
Repository
ignitionrobotics
Branch
default
Author
  1. Martin Pecka
Reviewers
Description

Implemented multiple FindFile and FindFileURI callbacks.

Inspired by https://bitbucket.org/osrf/gazebo/pull-requests/2948/support-custom-find-file-callbacks

Comments (7)

  1. Martin Pecka author

    I checked the Windows tests on my Windows machine and fixed them.

    During working on this, I got to re-thinking how the URIs are treated in ignition, and did some changes in the interpretation of URIs. These changes just shift Ignition closer towards https://tools.ietf.org/html/rfc2396 , but they're not needed if anybody thinks a URI should have a different meaning in Ignition.

    E.g.

    EXPECT_TRUE(path.Parse("/part 1/part 2/"));

    seems to be just wrong to me, since spaces in URIs should be encoded with either %20 or +.

    There are more blacklisted characters in the RFC, so I added all of them. This of course changes behavior of the URI class, and should thus be discussed if it is a change that's worth it. If not, I'll revert my changes in URI class and continue with only the SystemPaths changes.

    1. Louise Poubel

      Thanks for the changes. Allowing spaces on URIs is a recent addition, see pull request #110. Since we're not encoding URIs, that was needed in order to support URIs that have spaces, which is a common occurrence in Ignition Fuel (see this model for example).

      I agree that simply allowing spaces feels hacky, but without proper support for encoding, we need a way to support these URIs right now, so we can't just go back to forbidding spaces.

    2. Steven Peters

      I think it's better if we can keep the changes separate. Perhaps move the modifications to the URI class to a separate pull request? That will allow us to make progress on reviewing the FindFile and FindFileURI code