Skip to content

Commit

Permalink
Permit IO::Path given as path to use lib
Browse files Browse the repository at this point in the history 
Fixes RT#131540: https://rt.perl.org/Ticket/Display.html?id=131540
  • Loading branch information
@zoffixznet
zoffixznet committed Jun 9, 2017
1 parent f0c3bf7 commit 3ff29d4
Showing 1 changed file with 6 additions and 2 deletions.
8 changes: 6 additions & 2 deletions src/core/CompUnit/RepositoryRegistry.pm
View file
Expand Up @@ -12,9 +12,13 @@ class CompUnit::Repository::JavaRuntime { ... }
class CompUnit::RepositoryRegistry {
my $lock = Lock.new;

method repository-for-spec(Str $spec, CompUnit::Repository :$next-repo) {
proto method repository-for-spec(|) { * }
multi method repository-for-spec(IO::Path $spec, |c) {
self.repository-for-spec: $spec.absolute, |c;
}
multi method repository-for-spec(Str $spec, CompUnit::Repository :$next-repo) {
state %include-spec2cur;
state $lock = Lock.new;
# state $lock = Lock.new; # XXX TODO: RT#131542

my ($short-id,%options,$path) := parse-include-spec($spec);
my $class := short-id2class($short-id);
Expand Down

5 comments on commit 3ff29d4

@ugexe
Copy link
Member

@ugexe ugexe commented on 3ff29d4 Jun 9, 2017 •
edited

Choose a reason for hiding this comment

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

My first thought is I'm not sure this is the right way to handle this. Coercing to a string, maybe. But the interface takes strings because it's intended to accept things that can be passed on the command line or in env. Additionally different CURs can/could be used for absolute and relative paths, but the user cannot choose like this.

@zoffixznet
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So would you suggest moving the coersion to Actions that handle use lib instead?

@zoffixznet
Copy link
Contributor Author

@zoffixznet zoffixznet commented on 3ff29d4 Jun 9, 2017 •
edited

Choose a reason for hiding this comment

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

Actually, I don't entirely understand the concern. The IO::Path objects, even ones made from relative paths, will have CWD baked into them, so .absoluteing them even from another dir would still give the original path back. And the Str multi is still there for strings passed from the command line and env.

@ugexe
Copy link
Member

@ugexe ugexe commented on 3ff29d4 Jun 9, 2017 •
edited

Choose a reason for hiding this comment

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

Because the fact the paths are technically the same does not matter; what matters is the representation of it. We have a CUR::AbsolutePath and a CUR::FileSystem for instance. And now any third party CUR now must accept absolute paths if they want to work with IO::Path objects. absolutifying actually loses information (we no longer know the un-prefixed path).

Coercing to a string, assuming it coerces to the original string supplied by the user, avoids such issues. e.g.
sub foo(Str(Cool) $bar) { say $bar }; foo(IO::Path.new("/xxx")); foo(IO::Path.new("xxx/"))

@zoffixznet
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made changes in 326faed6c5

Please sign in to comment.