janet-lang / spork

Various Janet utility modules - the official "Contrib" library.
MIT License
122 stars 35 forks source link

added copy-file and directory handling helpers #75

Closed tionis closed 2 years ago

tionis commented 2 years ago

Should be similar in behaviour to mkdir -p.
This is a very naive implementation.

tionis commented 2 years ago

I also imported copy-file from my helper script collection

sogaiu commented 2 years ago

Regarding mkdir -p, here are a collection of implementations from various folks along those lines: https://gitlab.com/-/snippets/2054317

Regarding copying a file, here is what jpm does for reference: https://github.com/janet-lang/jpm/blob/b5cd98509931362f1bb0d3dd38a8ab637f329036/jpm/shutil.janet#L167-L178.

Note, it uses jpm's shell and a few other bits like is-win and path-splitter.

tionis commented 2 years ago

I see, I'll rework the code when I find the time

sogaiu commented 2 years ago

To clarfiy, my comment above wasn't implying the other implementations are better. Just sharing some other attempts :)

tionis commented 2 years ago

Thanks for the information, my implementations are quite simple, so I want to look at the other ones first.

tionis commented 2 years ago

OK, I read over the other implementations and added a small fix to create-dirs.
Regarding copy-file I'm not sure if shelling out is the best idea, but it seems to work quite fine for jpm, so maybe I should follow suite. I'm unsure about that and am open to suggestions.

tionis commented 2 years ago

Also copy-file only copies files, for a more general copy i would need to write a wrapper around it

sogaiu commented 2 years ago

Not sure if I'm doing something wrong, but evaluating the defmacro form for with-file gives me an error here.

Is the following the appropriate definition?

(defmacro with-file
  "Create and open a file, creating all the directories leading to the file if
  they do not exist, apply the given body on the file resource, and then close
  the file."
  [[binding path mode] & body]
  ~(do
    (def parent-path (,path/dirname ,path))
    (when (and (not (,exists? ,path)) (not (,exists? parent-path)))
      (,create-directories parent-path))
    (def ,binding (file/open ,path ,mode))
    ,(apply defer [:close binding] body)))

That's what I see in https://github.com/janet-lang/spork/pull/75/files/02327d7f3f6266eb7b25b62fb8454db89b91dbff

tionis commented 2 years ago

Ah, I see, I forgot to import my helper functions, I'm adding tests now.

tionis commented 2 years ago

I added some tests that should cover most but not all possible branches

tionis commented 2 years ago

This also tests the sh/rm function. I also wanted to add a test for exec-slurp but as it calls out to the OS I didn't want to introduce external test dependencies

bakpakin commented 2 years ago

Please use gensym in your with-file macro. Also not sure how general it is, perhaps it should be renamed - and I think you would be better served by something like (with [f (make-new-file)] …) instead of the macro.

tionis commented 2 years ago

OK, I replaced the macro with a function working in the manner you suggested.

tionis commented 2 years ago

I actually like this better, I don't remember why I had this as a macro

sogaiu commented 2 years ago

I tried jpm test with https://github.com/tionis/spork/commit/0715b5fcdc99a5019fcfb809935deab5addd6cbd and got an error:

running test/suite0017.janet ...
✘  sh/list-all-files didn't list the correct files

Perhaps the relevant code in the test file is:

  (assert (deep= (sh/list-all-files (path/join base-path "assets"))
                 (map |(path/join base-path $0)
                      @["assets/17/test.file" "assets/17/test2.file"]))
          "sh/list-all-files didn't list the correct files")

May be I'm doing something wrong?

tionis commented 2 years ago

That's weird, I just tried it out in three different environments and the tests are running through fine.
Including docker using following command:

docker run --rm -it tionis/janet sh -c 'git clone https://github.com/tionis/spork && cd spork && jpm test

Are you running on Windows? I haven't tested the code in windows yet.

tionis commented 2 years ago

I just pushed a commit that sorts the array before comparing the 2 to fix a possible inconsistency on that part.

sogaiu commented 2 years ago

I'm testing on a Linux box.

The latest commit https://github.com/tionis/spork/commit/a4bb8c496739b8bd60f9f36cf00c33b6c4925947 works fine here.

I retested with the earlier commit and it still fails so may be your change was effective (at least for my environment).