rejeep / f.el

Modern API for working with files and directories in Emacs
GNU General Public License v3.0
685 stars 69 forks source link

Add `f-presence`, `f-presence-directory`, `f-presence-file`, `f-presence-symlink`, and `f-presence-readable` #118

Closed sylvorg closed 1 year ago

sylvorg commented 1 year ago

Add functions to return path when path exists, is a file, is a directory, is a symlink, or is readable, similar to s-presence from Magnar's s.el.

This should be useful in situations such as (cond), where you might want to see if a file, directory, symlink, etc. exists before returning it, all in one go.

sylvorg commented 1 year ago

Also, I'm not quite sure how the shortdoc works, so if it would be great if I could get some help in changing that as well.

Phundrak commented 1 year ago

Hello, thank you for your PR! I would like some additional changes before merging.

First, could you add a -p equivalent to the ? predicates? Like f-file-present-p on top of f-file-present?. These better follows the EmacsLisp style than the ? predicates, which are more common in some other Lisp dialects like Scheme.

Secondly, as you mentioned it, there is no commit for the shortdoc file. For this, you can edit the f-shortdoc.el file. Below "Misc" line 323, you can add your new functions. Since they aren't pure functions (as they depend on the machine’s state), you should use :noevals as with f-empty-p, which gives an example of execution, and a :result on the next line giving an example of what the result might be. f-presence could have something like

(f-presence
 :noeval (f-presence-file "/path/to/existing/path")
 :result "/path/to/existing/path"
 :noeval (f-presence-file "/path/to/non/existing/path")
 :result nil)
Phundrak commented 1 year ago

I would also like you to modify README.org.tpl in order to update the documentation of the package.

sylvorg commented 1 year ago

Bah, humbug; the commit got slaughtered.

Otherwise, is this better? I didn't quite understand the tpl file, but I tried my best!

Sorry for the delay, though; had a final exam! :sweat_smile:

Phundrak commented 1 year ago

This PR seems almost good to go, I’d just like you to port the changes you made to README.org to README.org.tpl.

This is because the tpl file is the template for the README, the {{f-presence}}-like strings in the example blocks are replaced by the docstring of the function when generating the actual README.org file. The latter should be treated as a read-only file.

Sorry for the delay, though; had a final exam! :sweat_smile:

No worries, same here!

sylvorg commented 1 year ago

No worries, same here!

Hope you did well! 😸

Could you tell me what I missed? I thought I covered everything, already?

Phundrak commented 1 year ago

You’re actually good with the REAMDE, I don’t know why Github displayed the diff of an older commit.

I tried to run the tests on my machine, but quite a few of them fail. At first glance, it seems for instance you forgot to set a list as one, and you inverted the arguments of f-symlink for instance. You should rebase your branch on master or cherrypick https://github.com/rejeep/f.el/commit/859a6b8567e8d0cd9b0a71ac368af1152072b88d to fix the broken Cask file, run the tests locally (with make test), and fix them.

sylvorg commented 1 year ago

Ah, quick note: I rebased, and the Cask file seems fine, but running make test errors out with ert-runner: command not found. Also, where did I invert the arguments for f-symlink, exactly? I can't seem to find it in the presence functions, for example.

Phundrak commented 1 year ago

Ah, quick note: I rebased, and the Cask file seems fine, but running make test errors out with ert-runner: command not found.

Did you install the cask dependencies with cask install? I guess I should add it in the Makefile.

Also, where did I invert the arguments for f-symlink, exactly? I can't seem to find it in the presence functions, for example.

The f-symlink usage I mentioned is in the tests, in f-presence-symlink/symlink-paths. The signature of f-symlink is (f-symlink SOURCE PATH), but you wrote

(f-touch "file.txt")
(f-symlink "symlink" "file.txt")

Which tries to overwrite file.txt as a symlink to symlink, which does not exist. It should be (f-symlink "file.txt" "symlink") instead.

sylvorg commented 1 year ago

Hello! Sorry for the delay! Difficult uni course and all that! :sweat_smile: I also didn't realize I hadn't replied!

Everything seems to be in working order now!

Phundrak commented 1 year ago

Hi, thanks for updating the PR!

Hello! Sorry for the delay! Difficult uni course and all that!

Don’t sweat it, I’ve been there too 😄

It seems like there is still a rebase conflict with f-hidden-p’s definition, you may have rebased your branch on an outdated version of the repository. Could you rebase yourself on master?

Also, there’s no need to update the README.org file, it’ll update itself once the PR is merged (which is only blocked by the merge conflict).

It otherwise looks fine to me!

sylvorg commented 1 year ago

Interesting... Could you explain what the rebase actually did? I saw that the files from your master and my branch seem to have been the same either way? Why didn't pulling work in this case? Sorry, still relatively new to git. 😅

Phundrak commented 1 year ago

There are unfortunately quite a few changes between your branch and the master branch, you can see it by going on the Files changed tab of this PR. If you go on your repository, you can see your branch is 47 commits ahead of this repository while also missing 42 commits. Rebasing allows you to bring in your branch commits without the need of an extra merging commit. In your case, you somehow managed to modify commits so you are the committer (though the original author is kept), I’m not sure how you achieved this.

To properly rebase your branch, you need to first make sure whether you have this repository in your remotes or not, you can check it with git remote which will list all remotes you have in your repository, and you can check which repository each one points at using git remote get-url <branch-name>. You can check all of them at once with:

for remote in $(git remote); do
    echo $remote: $(git remote get-url $remote)
done

If you don’t have a remote pointing to this repository, you can add it with git remote add rejeep https://github.com/rejeep/f.el.git which will add the remote named rejeep pointing here.

Let’s say you have the remote rejeep pointing at this repository while origin points at yours. This means you have technically three master branches:

In order to properly synchronize your master branch (local and remote) with this repository’s, you can pull and rebase your local repository on this repository’s branch. This can be done by checking out your master branch with git checkout master and then pulling with git pull rejeep master --rebase. You can then checkout your development branch (in your case f-presence) and rebase it on your local master branch with git rebase master. You may have a merge conflict when you do that, you’ll have to resolve it. Once it is resolved, you can proceed with the rebase with git rebase --continue.

sylvorg commented 1 year ago

... What just happened? Did I just undo all my work?

Good thing I have a backup! 😹 Should I just apply the changes directly to a new fork off the master branch here?