Pass absolute paths to custom Builders #68

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
@raydiak
Contributor

raydiak commented Feb 19, 2014

Fixes problem where we chdir before calling the builder, and then pass it the path relative to the old (pre-chdir) working directory

@tadzik

This comment has been minimized.

Show comment Hide comment
@tadzik

tadzik Feb 19, 2014

Owner

Good catch, but I think I'd rather change $where where it comes from, as it's passed not only to build(), but also to test() install() and all the others. I think it's sane that all of them just get an absolute $where in the first place.

Owner

tadzik commented Feb 19, 2014

Good catch, but I think I'd rather change $where where it comes from, as it's passed not only to build(), but also to test() install() and all the others. I think it's sane that all of them just get an absolute $where in the first place.

@FROGGS

This comment has been minimized.

Show comment Hide comment
@FROGGS

FROGGS Feb 19, 2014

Collaborator

@raydiak I would suggest that you also pass it around as an IO::Path object instead of a string, so nobody needs to call .path on it again to get the IO::Path.

It would also be nice to add some documentation how Build.pm is meant to work, and that method build is called with an IO::Path object which holds an absolute path to the working directory. (In case there is no such doc yet)

Collaborator

FROGGS commented Feb 19, 2014

@raydiak I would suggest that you also pass it around as an IO::Path object instead of a string, so nobody needs to call .path on it again to get the IO::Path.

It would also be nice to add some documentation how Build.pm is meant to work, and that method build is called with an IO::Path object which holds an absolute path to the working directory. (In case there is no such doc yet)

@raydiak

This comment has been minimized.

Show comment Hide comment
@raydiak

raydiak Feb 19, 2014

Contributor

Agreed, I will look at where $where comes from and adjust accordingly some time tomorrow.

I also agree that it ought to be a path instead of a string, but wasn't sure if such a change would have other consequences. For example, are there likely to be modules in the wild already using a custom builder which have $where explicitly typed as Str?

Contributor

raydiak commented Feb 19, 2014

Agreed, I will look at where $where comes from and adjust accordingly some time tomorrow.

I also agree that it ought to be a path instead of a string, but wasn't sure if such a change would have other consequences. For example, are there likely to be modules in the wild already using a custom builder which have $where explicitly typed as Str?

@tadzik

This comment has been minimized.

Show comment Hide comment
@tadzik

tadzik Feb 19, 2014

Owner

On 02/19/2014 10:25 AM, raydiak wrote:

Agreed, I will look at where $where comes from and adjust accordingly
some time tomorrow.

I also agree that it ought to be a path instead of a string, but
wasn't sure if such a change would have other consequences. For
example, are there likely to be modules in the wild already using a
custom builder which have $where explicitly typed as Str?

Good point.

I guess we can try {} passing in a Path object, and look if it throws an
invalid argument type exception; if it does, we can then coerce the Path
to the Str and issue a deprecation notice.

Owner

tadzik commented Feb 19, 2014

On 02/19/2014 10:25 AM, raydiak wrote:

Agreed, I will look at where $where comes from and adjust accordingly
some time tomorrow.

I also agree that it ought to be a path instead of a string, but
wasn't sure if such a change would have other consequences. For
example, are there likely to be modules in the wild already using a
custom builder which have $where explicitly typed as Str?

Good point.

I guess we can try {} passing in a Path object, and look if it throws an
invalid argument type exception; if it does, we can then coerce the Path
to the Str and issue a deprecation notice.

@raydiak

This comment has been minimized.

Show comment Hide comment
@raydiak

raydiak Feb 20, 2014

Contributor

This is looking a little more involved than I had assumed. If we want paths to be consistently IO::Paths instead of Strs, changes will be required in several places, unless I've missed something obvious. I'd rather do it properly soon than poorly now, but I'm likely to get to it within another day or two.

Contributor

raydiak commented Feb 20, 2014

This is looking a little more involved than I had assumed. If we want paths to be consistently IO::Paths instead of Strs, changes will be required in several places, unless I've missed something obvious. I'd rather do it properly soon than poorly now, but I'm likely to get to it within another day or two.

@FROGGS

This comment has been minimized.

Show comment Hide comment
@FROGGS

FROGGS Feb 21, 2014

Collaborator

I'd think that this is all what is needed (except for the try {} block tadzik mentioned):

diff --git a/lib/Panda.pm b/lib/Panda.pm
index 2cc21ff..07cafc8 100644
--- a/lib/Panda.pm
+++ b/lib/Panda.pm
@@ -9,7 +9,7 @@ use JSON::Tiny;

 sub tmpdir {
     state $i = 0;
-    ".work/{time}_{$i++}"
+    ".work/{time}_{$i++}".path.absolute
 }

 class Panda {
diff --git a/lib/Panda/Common.pm b/lib/Panda/Common.pm
index 092fbf7..43c3f7c 100644
--- a/lib/Panda/Common.pm
+++ b/lib/Panda/Common.pm
@@ -5,9 +5,9 @@ sub dirname ($mod as Str) is export {
     $mod.subst(':', '_', :g);
 }

-sub indir (Str $where, Callable $what) is export {
+sub indir ($where, Callable $what) is export {
     mkpath $where;
-    temp $*CWD = IO::Spec.rel2abs($where);
+    temp $*CWD = $where.path.absolute;
     $what()
 }

Is Math::ThreeD supposed to fail tests?

Collaborator

FROGGS commented Feb 21, 2014

I'd think that this is all what is needed (except for the try {} block tadzik mentioned):

diff --git a/lib/Panda.pm b/lib/Panda.pm
index 2cc21ff..07cafc8 100644
--- a/lib/Panda.pm
+++ b/lib/Panda.pm
@@ -9,7 +9,7 @@ use JSON::Tiny;

 sub tmpdir {
     state $i = 0;
-    ".work/{time}_{$i++}"
+    ".work/{time}_{$i++}".path.absolute
 }

 class Panda {
diff --git a/lib/Panda/Common.pm b/lib/Panda/Common.pm
index 092fbf7..43c3f7c 100644
--- a/lib/Panda/Common.pm
+++ b/lib/Panda/Common.pm
@@ -5,9 +5,9 @@ sub dirname ($mod as Str) is export {
     $mod.subst(':', '_', :g);
 }

-sub indir (Str $where, Callable $what) is export {
+sub indir ($where, Callable $what) is export {
     mkpath $where;
-    temp $*CWD = IO::Spec.rel2abs($where);
+    temp $*CWD = $where.path.absolute;
     $what()
 }

Is Math::ThreeD supposed to fail tests?

@raydiak

This comment has been minimized.

Show comment Hide comment
@raydiak

raydiak Feb 21, 2014

Contributor

FROGGS++ has volunteered to take this issue off my hands as tuits are limited on my end atm

Contributor

raydiak commented Feb 21, 2014

FROGGS++ has volunteered to take this issue off my hands as tuits are limited on my end atm

@raydiak raydiak closed this Feb 21, 2014

@FROGGS

This comment has been minimized.

Show comment Hide comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment