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

suggestion: overload `/` for pathlib-like path construction #110

Closed
ClaytonJY opened this Issue May 26, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@ClaytonJY
Copy link

ClaytonJY commented May 26, 2018

I've been using python a lot lately and have come to love the intuitiveness of the division operator in pathlib:

import pathlib

parent = pathlib.Path('parent')

parent / 'child'
## PosixPath('parent/child')

I think fs is the closest analog to pathlib, and similar overloading could be implemented like this:

library(fs)

parent <- path("somedir")

# base errors
parent / "somefile"
#> Error in parent/"somefile": non-numeric argument to binary operator
parent + "somefile"
#> Error in parent + "somefile": non-numeric argument to binary operator

Ops.fs_path <- function(parent, child) {
  if (.Generic == "/") {
    path_join(c(parent, child))
  } else {
    NextMethod(.Generic, parent)
  }
}

# works!
parent / "somefile"
#> somedir/somefile
class(parent / "somefile")
#> [1] "fs_path"   "character"

# also works
parent / 1
#> somedir/1

# not quite same as before
parent + "somefile"
#> Error in `+.default`(parent, "somefile"): non-numeric argument to binary operator

(I suspect NextMethod should be something else to more exactly preserve the base error message, but I'm not sure what; my OO-foo is weak. I just know something needs to be there to prevent NULLs being returned for each unhandled generic.)

Is this worthy of inclusion in fs? If so I'd gladly submit a PR with tests and such.

@jimhester jimhester added the feature label Jun 3, 2018

@jimhester

This comment has been minimized.

Copy link
Member

jimhester commented Jun 3, 2018

You can actually just define the /.fs_path method directly, you don't have to define the entire group generic.

#' @export
`/.fs_path` <- function(e1, e2) {
  path(e1, e2)
}

I guess this also begs the question if we should define +.fs_path to concatenate onto a path, e.g.

path_home() / letters[1:3] / "baz" + ".txt"
#> /Users/jhester/a/baz.txt /Users/jhester/b/baz.txt /Users/jhester/c/baz.txt

My main reservation with this in general is traditionally R string based objects do not overload mathematical operators in this way, however I do agree the resulting syntax is nice.

@ClaytonJY

This comment has been minimized.

Copy link
Author

ClaytonJY commented Jun 4, 2018

ha so I struggled a bit trying to get something like

`/`.fs_path <- function

and figured the problem was using a base operator; didn't think to try moving the backtick way over there! Definitely much simpler.

I also see now why you'd use path over path_join here; the vectorization there is nice.

Would the + implementation be like this?

`+.fs_path` <- function(e1, e2) {
  paste0(e1, e2)
}

I understand your reservation but also love that clean syntax. Aside from breaking with tradition, since it's an fs_path-specific method, is it safe to assume this shouldn't break anyone's existing code?

Let me know which of these two (if any) you'd like to see, and I can take a stab at a PR.

@jimhester jimhester closed this in 318301f Jun 7, 2018

@jimhester

This comment has been minimized.

Copy link
Member

jimhester commented Jun 7, 2018

Ok I have added this. I am still somewhat unsure if there are unforeseen consequences, so it is somewhat experimental for now and won't be widely advertised in examples until we are more confident about it.

@ClaytonJY

This comment has been minimized.

Copy link
Author

ClaytonJY commented Jun 7, 2018

Excellent, thanks Jim! Hope they work smoothly; I've already started using them, but not for anything remotely complex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment