Skip Menu |
 

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the Path-Class CPAN distribution.

Maintainer(s)' notes

I prefer that bugs & patches are filed on GitHub rather than on RT: https://github.com/kenahoo/Path-Class/issues. Thanks.

Report information
The Basics
Id: 53066
Status: open
Priority: 0/
Queue: Path-Class

People
Owner: Nobody in particular
Requestors: schwern [...] pobox.com
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)

Attachments
0001-Fix-children-to-look-for-dot-dirs-in-a-cross-platfor.patch
0002-Make-dir-skip-itself-and-its-parent.patch



Subject: It would be useful if next() skipped . and ..
Date: Thu, 24 Dec 2009 13:59:23 -0800
To: bug-Path-Class [...] rt.cpan.org
From: Michael G Schwern <schwern [...] pobox.com>
$dir->next returns itself and its parent directory. While that's complete and all, is that ever useful? It would be far more useful to have an iterator that strips them out. For the odd cases you really want the complete directory listing you can use ->open and read from the handle. There's two patches attached. The first implements _is_dot_dir() which looks for . and .. in a cross-platform manner. That should be applied regardless of this ticket to fix children(). The second implements and tests the above. I thought about adding in an %opts parameter like children() but I'll leave that for somebody else or a future itch. I suppose this is backwards incompatible, but its so slight I can't get worked up about it. -- I'm pale as Formica, social skills stunted small. But I'm accurate like a pica, I know the capital of Nepal. I'm the nemesis of error, dreadful diction fears my skills, more inquisitive than Jim Lehrer, snottier than Beverly Hills. -- I.L.O.P. Secret Rap http://goats.com/archive/020830.html
From 5adac76146c5192adf947091e57a8ffff0a664f7 Mon Sep 17 00:00:00 2001 From: Michael G. Schwern <schwern@pobox.com> Date: Thu, 24 Dec 2009 13:51:40 -0800 Subject: [PATCH 1/2] Fix children() to look for dot dirs in a cross platform manner. --- lib/Path/Class/Dir.pm | 15 +++++++++++++-- lib/Path/Class/Entity.pm | 2 +- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/lib/Path/Class/Dir.pm b/lib/Path/Class/Dir.pm index 615b14b..4016d9e 100644 --- a/lib/Path/Class/Dir.pm +++ b/lib/Path/Class/Dir.pm @@ -11,6 +11,9 @@ use base qw(Path::Class::Entity); use IO::Dir (); use File::Path (); +my $Updir = __PACKAGE__->_spec->updir; +my $Curdir = __PACKAGE__->_spec->curdir; + sub new { my $self = shift->SUPER::new(); @@ -174,8 +177,7 @@ sub children { my @out; while (defined(my $entry = $dh->read)) { - # XXX What's the right cross-platform way to do this? - next if (!$opts{all} && ($entry eq '.' || $entry eq '..')); + next if !$opts{all} && $self->_is_dot_dir($entry); next if ($opts{no_hidden} && $entry =~ /^\./); push @out, $self->file($entry); $out[-1] = $self->subdir($entry) if -d $out[-1]; @@ -183,6 +185,15 @@ sub children { return @out; } +sub _is_dot_dir { + my $self = shift; + my $dir = shift; + + return 1 if $dir eq $Updir; + return 1 if $dir eq $Curdir; + return 0; +} + sub next { my $self = shift; unless ($self->{dh}) { diff --git a/lib/Path/Class/Entity.pm b/lib/Path/Class/Entity.pm index b858636..c790feb 100644 --- a/lib/Path/Class/Entity.pm +++ b/lib/Path/Class/Entity.pm @@ -39,7 +39,7 @@ sub new_foreign { return $class->new(@_); } -sub _spec { $_[0]->{file_spec_class} || 'File::Spec' } +sub _spec { (ref $_[0] && $_[0]->{file_spec_class}) || 'File::Spec' } sub boolify { 1 } -- 1.6.5.3
From cf355b17f31ad5ceb197491c5381153576dba0a5 Mon Sep 17 00:00:00 2001 From: Michael G. Schwern <schwern@pobox.com> Date: Thu, 24 Dec 2009 13:55:55 -0800 Subject: [PATCH 2/2] Make dir() skip itself and its parent. --- lib/Path/Class/Dir.pm | 13 ++++++++++--- t/03-filesystem.t | 6 ++++-- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/lib/Path/Class/Dir.pm b/lib/Path/Class/Dir.pm index 4016d9e..901178f 100644 --- a/lib/Path/Class/Dir.pm +++ b/lib/Path/Class/Dir.pm @@ -200,10 +200,14 @@ sub next { $self->{dh} = $self->open or Carp::croak( "Can't open directory $self: $!" ); } - my $next = $self->{dh}->read; + my $next; + do { + $next = $self->{dh}->read; + } while defined $next and $self->_is_dot_dir($next); + unless (defined $next) { - delete $self->{dh}; - return undef; + delete $self->{dh}; + return undef; } # Figure out whether it's a file or directory @@ -603,6 +607,9 @@ If an error occurs when opening the directory (for instance, it doesn't exist or isn't readable), C<next()> will throw an exception with the value of C<$!>. +Like children(), dir() will not include the I<self> and I<parent> +entries C<.> and C<..> (or their equivalents on non-Unix systems). + =item $dir->recurse( callback => sub {...} ) Iterates through this directory and all of its children, and all of diff --git a/t/03-filesystem.t b/t/03-filesystem.t index 5138ca3..2618550 100644 --- a/t/03-filesystem.t +++ b/t/03-filesystem.t @@ -4,7 +4,7 @@ use Test::More; use File::Temp qw(tmpnam tempdir); use File::Spec; -plan tests => 72; +plan tests => 74; use_ok 'Path::Class'; @@ -88,7 +88,9 @@ ok !-e $dir, "$dir no longer exists"; while (my $file = $dir->next) { push @contents, $file; } - is scalar @contents, 5; + is @contents, 3; + is_deeply [grep { $_ eq $dir } @contents], []; + is_deeply [grep { $_ eq $dir->parent } @contents], []; my $joined = join ' ', sort map $_->basename, grep {-f $_} @contents; is $joined, '0 file.x'; -- 1.6.5.3
Thanks.  I added a couple of comments for $Updir and $Curdir to note that they shouldn't respect "foreign" syntax, and changed the method name to _is_local_dot_dir() accordingly.  First patch applied.

I agree that the backcompat risk of the second patch is relatively small, but it's still nonzero.  I'd like to introduce it with a little more warning, e.g. adding an 'all' parameter that defaults to true, letting people set it to false - and declare that in a future release we'll change the default to false.  I'd be more than a little peeved if my program stopped working for an obscure little reason like that, with no warning.

I also think that the user options for next() ought to mirror the options for children().  Symmetry and all that.

In the meantime, this:

  while (my $e = $dir->next(all=>0)) {
     ... do something ...
   }

is a whole lot better than the current necessity:

  while (my $e = $dir->next) {
     next if $e eq File::Spec->curdir || $e eq File::Spec->updir;  # or hard-code
     ... do something ...
   }
 
From: dsteinbrunner [...] pobox.com
On Mon Dec 28 01:03:16 2009, KWILLIAMS wrote: Show quoted text
> > I agree that the backcompat risk of the second patch is relatively small, but > it's still nonzero. I'd like to introduce it with a little more warning, e.g. > adding an 'all' parameter that defaults to true, letting people set it to false > - and declare that in a future release we'll change the default to false. I'd > be more than a little peeved if my program stopped working for an obscure > little reason like that, with no warning. > > I also think that the user options for next() ought to mirror the options for > children(). Symmetry and all that. > > In the meantime, this: > > while (my $e = $dir->next(all=>0)) { ... do something ... } > is a whole lot better than the current necessity: > > while (my $e = $dir->next) { > next if $e eq File::Spec->curdir || $e eq File::Spec->updir; # or > hard-code ... > do something ... }
Just thought I would chime in and say that I came across this ticket because I expected the same behavior as schwern described and a change to allow it, via param or not, would still be beneficial.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.