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



MIME-Version: 1.0
X-Spam-Status: No, hits=-0.0 required=8.0 tests=DK_SIGNED,DK_VERIFIED,SPF_PASS
X-Virus-Checked: Checked by ClamAV on 16.mx.develooper.com
Content-Type: multipart/mixed; boundary="------------060802090008070706020109"
Message-ID: <4B33E43B.9040504 [...] pobox.com>
Received: from la.mx.develooper.com (x1.develooper.com [207.171.7.70]) by diesel.bestpractical.com (Postfix) with SMTP id C50C24D8109 for <bug-Path-Class [...] rt.cpan.org>; Thu, 24 Dec 2009 16:59:33 -0500 (EST)
Received: (qmail 10823 invoked by uid 103); 24 Dec 2009 21:59:33 -0000
Received: from x16.dev (10.0.100.26) by x1.dev with QMQP; 24 Dec 2009 21:59:33 -0000
Received: from a-pb-sasl-sd.pobox.com (HELO sasl.smtp.pobox.com) (64.74.157.62) by 16.mx.develooper.com (qpsmtpd/0.80) with ESMTP; Thu, 24 Dec 2009 13:59:29 -0800
Received: from sasl.smtp.pobox.com (unknown [127.0.0.1]) by a-pb-sasl-sd.pobox.com (Postfix) with ESMTP id 872DBA9A09 for <bug-Path-Class [...] rt.cpan.org>; Thu, 24 Dec 2009 16:59:26 -0500 (EST)
Received: from a-pb-sasl-sd.pobox.com (unknown [127.0.0.1]) by a-pb-sasl-sd.pobox.com (Postfix) with ESMTP id 83381A9A08 for <bug-Path-Class [...] rt.cpan.org>; Thu, 24 Dec 2009 16:59:26 -0500 (EST)
Received: from [10.23.42.2] (unknown [69.64.236.3]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by a-pb-sasl-sd.pobox.com (Postfix) with ESMTPSA id 98C55A9A07 for <bug-Path-Class [...] rt.cpan.org>; Thu, 24 Dec 2009 16:59:25 -0500 (EST)
Delivered-To: cpan-bug+Path-Class [...] diesel.bestpractical.com
User-Agent: Thunderbird 2.0.0.23 (Macintosh/20090812)
Subject: It would be useful if next() skipped . and ..
Return-Path: <schwern [...] pobox.com>
Domainkey-Signature: a=rsa-sha1; c=nofws; d=pobox.com; h=message-id:date :from:mime-version:to:subject:content-type; q=dns; s=sasl; b=HUH 75Y6O6bccvYL64yZofdKmeZf0WAH4UOB0Zc4FHzYySduIFgC6GfwT8ILU2txZQMa J6UQb5BAzRy107dwc75lNkWNctaJY1GUEgi6qAHfZ0B4XQEiaodf7cUeUtm5OS8r pGzXsaqFwx6NyHqr1rcJHGU8Mf7H/kxGk1gNnTB0=
X-Original-To: bug-Path-Class [...] rt.cpan.org
X-Spam-Check-BY: 16.mx.develooper.com
Dkim-Signature: v=1; a=rsa-sha1; c=relaxed; d=pobox.com; h=message-id :date:from:mime-version:to:subject:content-type; s=sasl; bh=ddsj LJpaw5nuCsvLR+39BZyN8tY=; b=maxbYumLQ1hGk/9Xj32Mb9MaD4CPEBa+yOI1 J729fXQYtIp7zG3JiR77YWSjR+yTlcD3Bd9p8p7jdzgFoI0fPiPYYDD1nxZ5bXLO T+/aVCau5ePw5+3s2ztE5wLlldvqe2JpnW1KRX81XOYl0MfVbJUE+ECiQz63NJfw jxPxypA=
Date: Thu, 24 Dec 2009 13:59:23 -0800
X-Spam-Level: *
To: bug-Path-Class [...] rt.cpan.org
X-Pobox-Relay-ID: 9A4BB6E2-F0D7-11DE-B02B-465EBBB5EC2E-02258300!a-pb-sasl-sd.pobox.com
From: Michael G Schwern <schwern [...] pobox.com>
Content-Length: 0
content-type: text/plain; charset="utf-8"; format="flowed"
Content-Transfer-Encoding: 7bit
X-RT-Original-Encoding: ISO-8859-1
Content-Length: 1043
$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
content-type: text/x-diff; name="0001-Fix-children-to-look-for-dot-dirs-in-a-cross-platfor.patch"
content-disposition: inline; filename="0001-Fix-children-to-look-for-dot-dirs-in-a-cross-platfor.patch"
Content-Transfer-Encoding: 7bit
Content-Length: 1828
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
content-type: text/x-diff; name="0002-Make-dir-skip-itself-and-its-parent.patch"
content-disposition: inline; filename="0002-Make-dir-skip-itself-and-its-parent.patch"
Content-Transfer-Encoding: 7bit
Content-Length: 2064
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
MIME-Version: 1.0
In-Reply-To: <4B33E43B.9040504 [...] pobox.com>
X-Mailer: MIME-tools 5.427 (Entity 5.427)
Content-Disposition: inline
References: <4B33E43B.9040504 [...] pobox.com>
Content-Type: text/html; charset="UTF-8"
Message-ID: <rt-3.8.HEAD-21052-1261980196-1120.53066-0-0 [...] rt.cpan.org>
Content-Transfer-Encoding: binary
X-RT-Original-Encoding: utf-8
Content-Length: 1347
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 ...
   }
 
MIME-Version: 1.0
In-Reply-To: <rt-3.8.HEAD-21052-1261980196-1120.53066-0-0 [...] rt.cpan.org>
X-Mailer: MIME-tools 5.427 (Entity 5.427)
Content-Disposition: inline
References: <4B33E43B.9040504 [...] pobox.com> <rt-3.8.HEAD-21052-1261980196-1120.53066-0-0 [...] rt.cpan.org>
Content-Type: text/plain; charset="UTF-8"
Message-ID: <rt-3.8.HEAD-6246-1347646060-12.53066-0-0 [...] rt.cpan.org>
Content-Transfer-Encoding: binary
From: dsteinbrunner [...] pobox.com
X-RT-Original-Encoding: utf-8
Content-Length: 1094
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.