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 extension and stripExtension functions #246

Merged
merged 3 commits into from
Jan 15, 2021
Merged

Add extension and stripExtension functions #246

merged 3 commits into from
Jan 15, 2021

Conversation

s-ludwig
Copy link
Member

In contrast to std.path, these work with pure forward ranges and directly accept GenericPath and GenericPath.Segment(2).

Copy link
Contributor

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

It seems a bit odd to have this as a standalone function. This module seems to offer a few standalone functions that returns Path, and the rest of the functionalities are wrapped in Path. This one stands out as acting on a range of chars.

Edit: I missed the PR message. Still having them needing a file name is an odd requirement that might trip people up. And the interface of this module also becomes a bit more complicated.


Note that `filename` must be a pure file name and not a full path.
*/
auto extension(R)(R filename)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use R instead of auto return type for better documentation

@@ -189,6 +190,129 @@ string toNativeString(P)(P path)
}


/** Returns the file extension of a file, including the dot.

Note that `filename` must be a pure file name and not a full path.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why this function is not just part of Path ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because that would also mean in Segment2 and possibly Segment, including duplicate documentation, and skipping the range based overload (which ideally should be adjusted in Phobos, but anyway).

@s-ludwig
Copy link
Member Author

There is also relativeTo and relativeToWeb in the form of free-standing functions. Making it into a member function would mean that it is scattered over three classes, as well as a remaining range based free-standing function.

@Geod24
Copy link
Contributor

Geod24 commented Jan 15, 2021

There is also relativeTo and relativeToWeb in the form of free-standing functions.

But they both return a Path and operate on Paths.

Because that would also mean in Segment2 and possibly Segment

Yeah... The duplication here is really getting in the way. Is there any plan to unify those ?

@s-ludwig
Copy link
Member Author

Segment is supposed to be deprecated at some point, and eventually replaced by Segment2, so that shouldn't be a concern here. I just added an overload for Segment because it was no effort, but with the deprecation in mind, that could also be left out.

Still having them needing a file name is an odd requirement that might trip people up.

Missed this point. I agree that this inconsistency with std.path is a potential pitfall. However, I heavily disagree with the requirement itself - std.path is an utterly horrible module, mixing all path formats into a single set of semantics that work most of the time, but on the other hand are bound to introduce hidden bugs all over the place (similar to the first implementation in vibe.d). Assuming a certain kind of path separator for these two functions is an example of this, maybe not always resulting in a major issue, but the same goes for not recognizing path separators at all.

Anyway, let's say that this possible pitfall (could at least be mitigated using an assertion that catches misuse), as well as the fact that this introduces an overload resolution conflict with std.path, is severe enough. That would mean that the range based overload needs to be kept private and the API duplication issue becomes less pronounced. In that case using methods instead of functions would become acceptable.

Just to clarify where I stand w.r.t. the general issue of member functions - I agree with you that semantically member functions are usually preferable for anything that seems like it is a property or a direct action on an object. The fragmented API resulting from the UFCS approach makes it harder to follow the documentation and to get a grasp on the API as a whole. On the other hand (ignoring D's choice of private semantics for a moment), using free-standing functions for any operations that only need the public interface of an object, and restricting methods/properties to things that operate on the object's internals, often leads to a better decomposition of the code and to a cleaner object design.

Using only public APIs, these two functions fall into the latter category, but the fact that they look like properties (and are syntactically supposed to be used like such) puts them somewhere in-between.

@s-ludwig
Copy link
Member Author

With this converted to methods, I'm really unhappy with the name stripExtension, which sounds like it's modifying the segment itself. If std.path didn't awkwardly choose that name for a different purpose, I would have used baseName. Any alternative ideas other than extensionStripped or withoutExtension?

@PetarKirov
Copy link

PetarKirov commented Jan 15, 2021

withoutExtension or omitExtension sound like good options.

@s-ludwig
Copy link
Member Author

I was actually seriously reconsidering baseName in the meantime, but I'll go for withoutExtension then.

Also adds GenericPath.fileExtension as a shortcut for `path.head2.extension`.
@s-ludwig s-ludwig merged commit d141ce2 into master Jan 15, 2021
@s-ludwig s-ludwig deleted the path_extension branch January 15, 2021 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants