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

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

Closed
wants to merge 0 commits into from

Conversation

sylvorg
Copy link

@sylvorg sylvorg commented Dec 1, 2022

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
Copy link
Author

sylvorg commented Dec 1, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@sylvorg
Copy link
Author

sylvorg commented Dec 17, 2022

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! 😅

@Phundrak
Copy link
Collaborator

Phundrak commented Dec 28, 2022

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! 😅

No worries, same here!

@sylvorg
Copy link
Author

sylvorg commented Dec 28, 2022

No worries, same here!

Hope you did well! 😸

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

@Phundrak
Copy link
Collaborator

Phundrak commented Dec 29, 2022

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 859a6b8 to fix the broken Cask file, run the tests locally (with make test), and fix them.

@sylvorg
Copy link
Author

sylvorg commented Dec 29, 2022

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
Copy link
Collaborator

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.

@Phundrak Phundrak added this to the v0.22 milestone Jan 17, 2023
@sylvorg
Copy link
Author

sylvorg commented May 27, 2023

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

Everything seems to be in working order now!

@Phundrak
Copy link
Collaborator

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
Copy link
Author

sylvorg commented May 28, 2023

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
Copy link
Collaborator

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:

  • master: your local copy of the branch
  • origin/master: the master branch as it is on your repository
  • rejeep/master: the master branch on this repository

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
Copy link
Author

sylvorg commented May 30, 2023

... 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?

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

Successfully merging this pull request may close these issues.

None yet

2 participants