ruricolist / serapeum

Utilities beyond Alexandria
MIT License
420 stars 41 forks source link

pathname util suggestions #127

Closed Ambrevar closed 1 year ago

Ambrevar commented 2 years ago

Too often I've been bitten by merge-pathnames* and even sera:path-join only fixes part of the sources of confusion.

For instance, you could argue the following does not do "what I mean":

> (sera:path-join #p"/path/to/basename" #p".ext")
; => #P"/path/to/.ext"

There are more pitfalls along these lines.

So I rolled out my own in https://github.com/atlas-engineer/nfiles/blob/89b4ecacfd9db9c78de48c2f096b8309f245bb45/pathname-helpers.lisp#L53:

(defun join (&rest paths)
  "Concatenate PATHS."
  (if (< (length paths) 2)
      (the (values pathname &optional)
           (uiop:ensure-pathname (first paths)))
      (apply #'join
             (let ((path1 (first paths))
                   (path2 (second paths)))
               (if (or (null (pathname-name path1))
                       (pathname-directory path2))
                   (uiop:merge-pathnames* (uiop:relativize-pathname-directory (uiop:ensure-pathname path2))
                                          (uiop:ensure-pathname path1
                                                                :ensure-directory t))
                   (let ((new-base (uiop:strcat (basename path1)
                                                (basename path2))))
                     (make-pathname :defaults path1 :type (pathname-type new-base)
                                    :name (pathname-name new-base)))))
             (cddr paths))))

Now all these tests pass:

(subtest "Join"
  (is (nfiles:join "foo")
      #p"foo"
      :test 'uiop:pathname-equal)
  (is (nfiles:join #p"foo" "bar")
      #p"foobar"
      :test 'uiop:pathname-equal)
  (is (nfiles:join #p"foo" "bar" #p"baz")
      #p"foobarbaz"
      :test 'uiop:pathname-equal)
  (is (nfiles:join #p"foo" "bar/baz")
      #p"foo/bar/baz"
      :test 'uiop:pathname-equal)
  (is (nfiles:join #p"foo/bar" "baz")
      #p"foo/barbaz"
      :test 'uiop:pathname-equal)
  (is (nfiles:join #p"foo/bar/" "baz")
      #p"foo/bar/baz"
      :test 'uiop:pathname-equal)
  (is (nfiles:join #p"foo/" "bar/" "baz" "qux")
      #p"foo/bar/bazqux"
      :test 'uiop:pathname-equal)
  (is (nfiles:join #p"foo.txt" "bar/baz")
      #p"foo.txt/bar/baz"
      :test 'uiop:pathname-equal)
  (is (nfiles:join #p"foo.txt" "bar.ext")
      #p"foo.txtbar.ext"
      :test 'uiop:pathname-equal))

What do you think of merging this into Serapeum?

By the way, nfiles/pathname-helpers has a few more utils that would be welcome in Serapeum in my opinion, such as

Well, have a look :)

ruricolist commented 2 years ago

These all look very useful. The only one I think is out of scope is file-user, since that relies on iolib and I try to keep Serapeum portable.

My only concern is that some of the names (parent, in particular) might be likely to lead to conflicts.

Ambrevar commented 2 years ago

Conflicts are then easily avoided by using the same strategy that you did with path-join, just prefix it with path- :) So here path-parent.

It's not just file-user, it has other OS-related functions: file-group, permissions. And they are setf-able! Note that all these OS-related functions don't depend on IOlib with SBCL. I'm guessing that it would be possible to write similar implementation specific code to remove the need for IOlib with other compilers.

What about making these non-portable functions an Serapeum contrib?

ruricolist commented 2 years ago

Keeping the non-portable functions in a contrib sounds like a good idea. They could be moved into Serapeum proper if and when they can be rewritten portably. (Although something like portable file permissions might deserve its own library.)

Ambrevar commented 2 years ago

Feel free to copy-paste (and adapt) the code from Nfiles if you like it! :)

Ambrevar commented 2 years ago

On second thought, I wonder if this wouldn't be a better fit in UIOP. After all, if I'm not mistaken Serapeum has only one pathname helper.

Besides, cl:pathname are too hard to use by default and UIOP being available everywhere has really helped alleviate this problem. So if we added pathname-join and pathname-base to UIOP (addressing the 2 biggest pitfalls when using cl:pathname), it would hopefully hit the final nail on the cl:pathname coffin of bugs.

What do you think?

ruricolist commented 2 years ago

My understanding is that UIOP only includes what is directly needed by ASDF.

Ambrevar commented 2 years ago

Yes, that and also not all implementations (SBCL, ABCL...) update their ASDF, which means that by default we can't expect users to have the latest version.

So maybe Alexandria instead?

ruricolist commented 2 years ago

I think Alexandria inclusion would also be a long shot.

I am interested in including this in Serapeum. I have pushed a branch that integrates it (path-join-dwim) and will be doing some testing with it.

If it turns out that the change from the existing behavior is too great, I will include it under a different name (path-join-dwim) but I'd rather not end up maintaining a legacy path-join if I don't have to.

Ambrevar commented 2 years ago

Thanks!

Also note that I've made some changes to basename which is tougher to implement that I initially thought. The main question is whether basename should truenamize the input or not. Maybe provide 2 version, basename and basename*, where only the latter truenamizes?

Thoughts?

The other ones are trivial but very useful because they often bite the programmer: parent, pathname-type*, etc.

ruricolist commented 2 years ago

I've noticed a couple of practical differences in testing:

  1. Formerly if you did (path-join X Y) and X and Y were both absolute, Y would override X. Now Y gets turned into a relative suffix of X. I think this behavior is actually better and the old behavior is likely to hide bugs, so I'm willing to make a breaking change.
  2. There seems to be a difference in what happens with wild pathnames. This one I need to investigate more.
Ambrevar commented 2 years ago
  1. Yup, this is one of the main goals of this function.
  2. I haven't tested with wild pathnames, it needs more testing for those.
Ambrevar commented 1 year ago

@ruricolist Any update on this?

ruricolist commented 1 year ago

I'm inclined to include it mostly as is, but under a new name (path-concat).

Ambrevar commented 1 year ago

Fine with me :)

Ambrevar commented 1 year ago

What about basename and parent?

ruricolist commented 1 year ago

This is (finally) included as Serapeum as base-path-join.

Ambrevar commented 1 year ago

Thanks!