ocaml-multicore / eio

Effects-based direct-style IO for multicore OCaml
Other
529 stars 67 forks source link

Add Path.{basename,dirname} #648

Closed mbarbin closed 6 months ago

mbarbin commented 8 months ago

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 commented 6 months ago

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.

talex5 commented 6 months ago

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.