Add change_extension utility method to rename the file extension #292
Conversation
… file Motivated by C# Path.ChangeExtension method
There was a problem hiding this comment.
Hi! Thanks for the PR. This is a really cool feature that I'd be interested in adding to Path::Tiny, but there's a lot of work to do before I consider it ready to merge. Please take this amount of feedback as a positive sign -- I tend to be very stingy about accepting new features and close most submissions as "Won't Do". I wouldn't take the time to review in this depth if I didn't think it had promise.
| my $new_extension = shift; # may be undef | ||
|
|
||
| my $path_str = $self->stringify; | ||
| $path_str =~ s/\.[^.\/]+$//; # Remove existing extension |
There was a problem hiding this comment.
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.
| $path_str =~ s/\.[^.\/]+$//; # Remove existing extension | ||
|
|
||
| # If extension is undef, the returned string contains the specified path with its extension removed. | ||
| return path($path_str) unless defined $new_extension; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
lib/Path/Tiny.pm
Outdated
| $new_extension = '.' . $new_extension; | ||
| } | ||
|
|
||
| $path_str .= $new_extension; # Add new extension |
There was a problem hiding this comment.
Very small nit: This is so trivial that you can do it as part of L2455.
lib/Path/Tiny.pm
Outdated
| $path_str .= $new_extension; # Add new extension | ||
|
|
||
| # Construct the new path | ||
| my $new_path = path($path_str); |
There was a problem hiding this comment.
Use _path and return this directly rather than creating a temporary variable to return on L2457.
| Returns a Path::Tiny object. | ||
|
|
||
| If extension is C<undef>, the returned string contains the specified path with its extension removed. | ||
| If path has no extension, and extension is not C<undef>, the returned path string contains extension appended to the end of path. |
There was a problem hiding this comment.
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.
t/change_extension.t
Outdated
|
|
||
| { | ||
| my $renamed1 = path("C:/mydir/myfile.com.extension")->change_extension(".old"); | ||
| ok($renamed1->stringify eq 'C:/mydir/myfile.com.old', 'rename to .old with leading perdiod'); |
There was a problem hiding this comment.
I strongly prefer table-driven tests because it's much easier to extend with new cases. See t/subsumes.t for an example as well. Here, you've grouped tests inside brackets, which is similar to how tests are grouped in subsumes.t.
t/change_extension.t
Outdated
| if ($@) { | ||
| $died = 1; | ||
| } | ||
| ok($died, 'Remove extension from file starting with period (and no further etxension) dies as expected'); |
There was a problem hiding this comment.
Testing that something dies isn't enough. If there is an exception, I prefer to verify that the message is appropriate to the situation using the exception utility that you can see in many files, such as t/size.t. That said, as I said in another comment, I don't think change_extension should be fatal.
t/change_extension.t
Outdated
|
|
||
| { | ||
| my $dir1 = path("C:/mydir/lookslikedorectory")->change_extension(undef); | ||
| ok($dir1->stringify eq 'C:/mydir/lookslikedorectory', 'directory names without period are kept when removing suffix'); |
There was a problem hiding this comment.
Except in very specific cases, Path::Tiny does not distinguish between "files" and "directories" because it works on paths without reference to the filesystem. These test are effectively "path without extension" tests. They are valuable, but please don't refer to them as directories.
There was a problem hiding this comment.
Good point. I actually already changed the POD too to only use "path". The test cases are already adjusted (the other changes still pending).
t/change_extension.t
Outdated
| my $dir2 = path("C:/mydir/lookslikedirectory")->change_extension(".exten"); | ||
| ok($dir2->stringify eq 'C:/mydir/lookslikedirectory.exten', 'directory names are extended when adding suffix'); | ||
|
|
||
| my $dir3 = path("C:/mydir/lookslikedirectory")->change_extension("exten"); |
There was a problem hiding this comment.
Final test note: all your paths lead with C: so are windows specific and absolute file path specific. See t/subsumes.t for a wider variety of cases to test, including unix-style, relative vs absolute, with and without directory separators, and special cases like . and ... All such cases need to be covered by these tests.
Except in very specific cases, Path::Tiny does not distinguish between "files" and "directories" because it works on paths without reference to the filesystem. Source: dagolden#292 (comment)
First iteration, might need more work. Cf. dagolden#292 (comment)
Makes it easier to extend with new cases. Cf. dagolden#292 (comment)
Motivated by C# Path.ChangeExtension method