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 Path.{basename,dirname} #648

Closed
wants to merge 2 commits into from
Closed

Conversation

mbarbin
Copy link

@mbarbin mbarbin commented Nov 17, 2023

Add Path.basename and Path.dirname as convenience wrappers around Path.split.

I had originally thought it would be useful for these functions not to return an option, but later changed my mind. I updated the PR to match @talex5's recommendation in #642:

I think basename etc should just be e.g. snd (split t).

As it turns out, I now only uses split in the piece of code that I was migrating to eio that prompted my original feedback, thus I am not confident whether these wrappers are useful. Please feel free to close the PR. Thank you!

@mbarbin mbarbin changed the title Add Path.{basename,dirname,parent_dir} Add Path.{basename,dirname} Dec 8, 2023
@mbarbin
Copy link
Author

mbarbin commented Dec 31, 2023

Hi @talex5 and team,

I want to express my gratitude for your time and the thoughtful feedback you've
provided on issue #642. It's been a valuable learning experience for me.

After further consideration and usage of the split function in my code, I've
come to realize that the basename and dirname convenience wrappers may not
be the best match for the code I was migrating to Eio. The split function
provides sufficient functionality for my current needs.

I am also having second thoughts on implementing dirname as a simple wrapper
using snd, as it can potentially return an empty string, which is an invalid
path. This can cause functions like Fpath.of_string to fail. It's worth noting
too that this behavior diverges from the Unix commands dirname, which might be
surprising:

$ dirname ' '
.
$ dirname '/'
/
$ dirname '.'
.

I believe this could potentially lead to confusion and unexpected bugs. As a
data point, I wasn't careful enough and the very first attempt I made to using
dirname resulted in a runtime error:
Fpath.of_string raising Invalid_argument \"\": invalid path.

I want to clarify that my feedback in #642 was based on my personal experience
and the specific use case I was working on. I'm still exploring the best way to
articulate what I felt was missing regarding path manipulation, and I hope to
provide more constructive feedback in the future, perhaps opening a more
specific issue, TBD.

Given these reasons, I've decided to close this PR. However, I don't want this
to discourage anyone else who might find wrappers of this kind useful from
creating another PR. I believe that different use cases might warrant different
solutions, and this might be beneficial to someone else with other motivating
examples.

Thank you for your time and for the great work you're doing on the Eio library.
I look forward to discussing more in the future.

@mbarbin mbarbin closed this Dec 31, 2023
@talex5
Copy link
Collaborator

talex5 commented Jan 5, 2024

Thanks - that's really useful feedback. It does sound like we need to wait until we have a use for these functions before deciding how they should behave.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants