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

Inherent methods for PathOps and PathInfo #48

Open
ExpHP opened this issue Jul 16, 2019 · 6 comments
Open

Inherent methods for PathOps and PathInfo #48

ExpHP opened this issue Jul 16, 2019 · 6 comments

Comments

@ExpHP
Copy link

ExpHP commented Jul 16, 2019

It feels extremely silly to have to import traits to get methods that could sensibly be provided on the types themselves, especially traits that have so many methods (PathInfo), and especially when I have no intention to use the traits generically. (I already have my own trait with a method named join, and importing yours would cause a conflict)

Most notably:

  • {PathAbs,PathDir}::{join,concat}
  • {PathAbs,PathFile}::{file_name,with_file_name,with_extension} (and maybe stuff for PathDir; though the term base_name might be cause less confusion than file_name)
  • PathAbs::{exists,is_file,is_dir,parent...}... in fact, nearly all of the Path methods are useful for PathAbs, making it really hard for me to see why you removed impl Deref for PathAbs. Maybe Target=PathArc was too much, but isn't at least Target=Path reasonable?
@vitiral
Copy link
Owner

vitiral commented Jul 16, 2019

I already have my own trait with a method named join, and importing yours would cause a conflict

I'm curious: is it implemented for Path? What does it do that is different than what PathInfo::join does?

The reason we moved these to traits is so that these methods could be used on Path and PathBuf, removing most of the major differences. It allows for using either type much more easily.

@ExpHP
Copy link
Author

ExpHP commented Jul 16, 2019

It does the same as PathInfo::join. (well, PathInfo::join doesn't exist, but I know what you mean)

It has been around in my crate since before I started using path_abs. It's mostly similar to AsRef<Path>, being implemented on Path, str, temp_dir::TempDir, etc. But there was a time when I needed such a trait implemented on Rc<TempDir> so that I could turn one into a trait object. (the object was dynamically either a PathBuf or an Rc<TempDir>). Such an impl is impossible with AsRef<Path> (as well as with PathInfo), hence why I created the trait.

Currently, the trait permeates a great portion of my codebase and I would not look forward to determining whether each use site needs PathOps, PathInfo, or both.

use std::path::Path;
use std::path::PathBuf;

/// AsRef<Path> with more general impls on smart pointer types.
///
/// (for instance, `Box<AsPath>` and `Rc<TempDir>` both implement the trait)
pub trait AsPath {
    fn as_path(&self) -> &Path;

    fn to_path_buf(&self) -> PathBuf
    { self.as_path().to_path_buf() }

    fn join(&self, path: impl AsPath) -> PathBuf
    where Self: Sized, // generic functions are not object-safe
    { self.as_path().join(path.as_path()) }
}

macro_rules! as_path_impl {
    ---- (snip) ----
}

as_path_impl!{
    (by AsRef) [] std::path::Path;
    (by Deref) [] std::path::PathBuf;
    (by AsRef) [] rsp2_fs_util::ActualTempDir;
    (by AsRef) [] rsp2_fs_util::TempDir;
    (by AsRef) [] std::ffi::OsString;
    (by AsRef) [] std::ffi::OsStr;
    (by AsRef) [] str;
    (by AsRef) [] String;
    (by AsRef) ['a] std::path::Iter<'a>;
    (by AsRef) ['a] std::path::Components<'a>;
    (by Deref) ['p, P: AsPath + ?Sized] &'p mut P;
    (by Deref) [P: AsPath + ?Sized] Box<P>;
    (by Deref) [P: AsPath + ?Sized] std::rc::Rc<P>;
    (by Deref) [P: AsPath + ?Sized] std::sync::Arc<P>;
    (by Deref) ['p, P: AsPath + ToOwned + ?Sized] std::borrow::Cow<'p, P>;
    (by AsRef) [] path_abs::PathAbs;
    (by AsRef) [] path_abs::PathFile;
    (by AsRef) [] path_abs::PathDir;
}

impl<'p, P: AsPath + ?Sized> AsPath for &'p P
{ fn as_path(&self) -> &Path { P::as_path(self) } }

@vitiral
Copy link
Owner

vitiral commented Jul 16, 2019

Just so I understand the ask, are you proposing:

  1. path_abs doesn't have these traits (PathInfo, PathOps, PathMut)
  2. OR path_abs does have the traits, but also implements the methods for all the types it exports explicitly.

If it is the second, is there precedence for that in other libraries?

I empathize with the breaking changes causing problems. I also empathize that there is no way to implement PathInfo etc for types you don't own.

When working with TempDir myself I always use it's as_path() before doing anything with it, and that has worked out fine for me (works out better, now that the traits are implemented).

Personally I don't think it makes much sense to implement the traits for str. Do you really use that a lot, or is just a convenient shortcut? I'm so used to python's str.join that I think it would confuse me.

@ExpHP
Copy link
Author

ExpHP commented Jul 17, 2019

Option 2.

The trouble with finding existing examples is that if a library does it right, you don't notice. I opened this thread: https://users.rust-lang.org/t/crates-that-use-the-inherent-trait-method-pattern/30381

Personally I don't think it makes much sense to implement the traits for str. Do you really use that a lot, or is just a convenient shortcut? I'm so used to python's str.join that I think it would confuse me.

str is often used for the right-hand argument. It didn't actually occur to me that join could be called on str.

@vitiral
Copy link
Owner

vitiral commented Jul 18, 2019

But you can already use it for the RHS, since str is AsRef<Path>

https://github.com/vitiral/path_abs/blob/master/tests/test_absolute.rs#L49

@ExpHP
Copy link
Author

ExpHP commented Jul 19, 2019

Yes, granted, the RHS of join (and other functions that take relative paths in the crate) could be changed to AsRef<Path>, and the str impl could be removed. It's just easier to have one trait that I know is good for all "pathlike" args, rather than two.

My crate no longer needs the Rc<TempDir> -> Box<dyn Trait> usecase, so the trait is admittedly technical debt at this point that could someday be removed. If anything, I'm just not sure that I really want to "double down" on path_abs by switching to use its traits all over the code base.

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

No branches or pull requests

2 participants