Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add change_extension utility method to rename the file extension #292

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions lib/Path/Tiny.pm
Original file line number Diff line number Diff line change
Expand Up @@ -2415,6 +2415,46 @@ sub volume {
return $self->[VOL];
}

=method change_extension

my $foo = path('C:/mydir/myfile.com.extension');
my $renamed_foo = $foo->change_extension('.old');

Changes the extension of a path.

Returns a new Path::Tiny object with a different extension. The argument is a
string representing the new extension or undef to remove an extension.

If path has no extension, and extension is not C<undef>, the returned path string contains extension appended to the end of path.
Copy link
Contributor

@xdg xdg Jun 15, 2024

Choose a reason for hiding this comment

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

This paragraph is redundant with L2426 and overall the docs are wordy with lots of space. If you look at docs like for copy, you can see a tighter style. Docs for this function could be as concise as this:

Returns a new Path::Tiny object with a different extension.  The argument is a
string representing the new extension or undef to remove an extension.

You then might have an extra paragraph explaining the special case that a leading period is not considered an extension.

If the last part of a path has a leading period (e.g. C<~/.bashrc>), it is not considered an extension.

Current API available since 0.148.

=cut

sub change_extension {
my $self = shift;
my $new_extension = shift; # may be undef

my $path_str = $self->stringify;
$path_str =~ s/\.[^.\/]+$//; # Remove existing extension
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to address files where the leading character is a period. E.g. .bashrc. Perhaps you need to parse into a root part and an extension part and decide on behavior if either part is missing.

Update: I saw the tests where you expect this case to die. I think this should not be fatal, as people may iterate over a bunch of files and would be surprised to have leading-dot files throw exceptions instead of no-op the way other files without extensions behave. I.e. .foo should behave like foo.


# If extension is undef, the returned string contains the specified path with its extension removed.
return path($path_str) unless defined $new_extension;
Copy link
Contributor

Choose a reason for hiding this comment

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

For paths created by functions, use _path instead of path. See child and many other functions.

Also, if new_extension is the empty string, this will not return and the function will return ${path-str}.. Is that the best API? Might some people do path($original)->change_extension("") thinking that will remove an extension? I'm not sure either way. This requires some thinking and maybe discussions with people to sound out what they would expect.

Copy link
Author

Choose a reason for hiding this comment

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

I had exactly the same thoughts when implementing the feature but I didn't have additional opinions on this matter.
The reason why I did it in the way as proposed was somewhat random. It seemed to be closer to the way how it is done in C#. However, the Path class in C# is a static class and cannot be instantiated at all - unlike Path::Tiny.

I would like to propose that I change the implementation so that it mutates the calling path. Its imply feels better. In addition, the verb "change" when called as an objetc method IMO somewhat imples that it changes the object.


if( $new_extension !~ m/^\./ ) {
# add leading period if there is no period
$new_extension = '.' . $new_extension;
}

# Add extension and construct the new path
my $new_path = _path($path_str . $new_extension);

return $new_path;
}



package Path::Tiny::Error;

our @CARP_NOT = qw/Path::Tiny/;
Expand Down
67 changes: 67 additions & 0 deletions t/change_extension.t
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
use 5.008001;
use strict;
use warnings;
use Test::More 0.96;

use lib 't/lib';

use Path::Tiny;

my @cases = (
# path1 => path2 => path1->subsumes(path2)

"rename path with extension" => [
[ '.', '.ext', '.ext' ],
[ '/', '.ext', '/.ext' ],
[ '..', '.ext', '..ext' ],
[ '../..', '.ext', '../..ext' ],
[ '/foo/', '.ext', '/foo.ext' ], # differs from C#: /foo/.ext
[ '/foo', '.ext', '/foo.ext' ],
[ 'foo/', '.ext', 'foo.ext' ], # differs from C#: foo/.ext
[ './foo', '.ext', 'foo.ext' ], # differs from C#: ./foo.ext
[ 'foo/.', '.ext', 'foo.ext' ], # differs from C#: foo/.ext
[ 'C:/temp/myfile.com.extension', '.old', 'C:/temp/myfile.com.old' ],
[ 'C:/temp/myfile.com.extension', 'old', 'C:/temp/myfile.com.old' ],
[ 'C:/pathwithoutextension', '.old', 'C:/pathwithoutextension.old' ],
[ 'C:/pathwithoutextension', 'old', 'C:/pathwithoutextension.old' ],
# ~ paths
],

"remove extension" => [
[ '.', undef, '' ],
[ '/', undef, '/' ],
[ '..', undef, '.' ],
[ '../..', undef, '../.' ],
[ '/foo/', undef, '/foo' ], # differs from C#: /foo/
[ '/foo', undef, '/foo' ],
[ 'foo/', undef, 'foo' ], # differs from C#: foo/
[ './foo', undef, 'foo' ], # differs from C#: ./foo
[ 'foo/.', undef, 'foo' ], # differs from C#: foo/
[ 'C:/temp/myfile.com.extension', undef, 'C:/temp/myfile.com' ],
[ 'C:/temp/myfile.com.extension', undef, 'C:/temp/myfile.com' ],
[ 'C:/pathwithoutextension', undef, 'C:/pathwithoutextension' ],
[ 'C:/pathwithoutextension', undef, 'C:/pathwithoutextension' ],
],

);



while (@cases) {
my ( $subtest, $tests ) = splice( @cases, 0, 2 );

subtest $subtest => sub {
for my $t (@$tests) {
my ( $path1, $ext, $path2 ) = @$t;
my $label = sprintf("%s + %s -> %s", $path1, (defined $ext ? $ext : 'undef'), $path2);
my $changed_path = path($path1)->change_extension($ext);
ok( $changed_path->stringify eq $path2, $label )
or diag "PATH 1:\n", explain( path($path1) ), "\nCHANGED PATH:\n", explain( $changed_path ), "\nPATH2:\n",
explain( path($path2) );
}
};
}

ok(1);

done_testing;