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

Cleanup #80

Merged
merged 10 commits into from Aug 9, 2022
Merged

Cleanup #80

merged 10 commits into from Aug 9, 2022

Conversation

jeanphilippegg
Copy link
Contributor

@jeanphilippegg jeanphilippegg commented Aug 9, 2022

Many cleanups:

  • I have removed the let-bindings of default-directory. I had to be careful because let-binding default-directory can have non obvious consequences. Please double-check the code and test the affected functions: denote, denote-link-backlinks, denote--edit-front-matter-p (the renaming functions) and denote-link-add-links. My own tests did not yield any errors.

  • I deleted the unused variable denote-link--links-to-files.

  • I removed the check for the current-buffer in denote--id-exists-p. We already discussed this in another issue/pull request. We did not know what it was used for, but we kept it. denote--id-exists-p is only used by the denote command now. I don't know if it was used elsewhere previously, but I cannot imagine why this check would be necessary now. I say we remove it and if there is any issue, we fix it and document it. By the way, denote--id-exists-p has been broken for weeks (I fixed it. See below.).

  • The following functions were simplified:

    • denote--directory-files: Remove the absolute argument from denote--directory-files because we (almost) always set it anyway.
    • denote--directory-files-recursively: Merged it in denote--directory-files because it is now only a single line.
    • denote--buffer-file-names: Now returns absolute paths as well.
    • denote--retrieve-files-in-xrefs and denote--retrieve-proces-grep: Use absolute paths.

    The idea is to keep these functions simple and make them always return absolute paths. We can use denote--file-name-relative-to-denote-directory on the items of the list, when necessary, but it does not happen often (only once for each function).

  • In denote-subdirs, I removed the TODO items. I made it to ignore hidden directories. This way, a lot of VC backends are handled and we provide a way for users to create directories ignored by Denote in denote-subdirectory. What do you think of this?

Bugs that were noticed and fixed as part of these changes:

  • Calling (denote--id-regexp "some-existing-id") at the root of denote-directory triggers the debugger. This has probably gone unnoticed because it is only used to check collisions and it never happens.

  • In a note at the root of denote-directory, calling denote-link-add-links with a regex matching the current file does not add the current file to the links. Doing the same operation in a file inside a subdirectory adds the current file to the links.

  • A directory called some.notes.about.git is ignored by Denote because it contains .git.

- Clicking on a link in a file in a subdirectory should open the linked file.
@protesilaos
Copy link
Owner

Merged. Thank you!

@jeanphilippegg jeanphilippegg deleted the cleanup branch August 9, 2022 03:51
@protesilaos
Copy link
Owner

About the check for hidden directories: I think it should be okay this way, though it can still theoretically return some false positives. If there ever is a problem with it, we will need to exclude them specifically.

@protesilaos
Copy link
Owner

I think it is an elegant solution to let the user omit subdirectories by prepending the dot. Though I wonder if it could cause confusion with those users who alwyas have dotfiles hidden. A user option is more fool-proof in that regard, though I do not consider it important to add right now. Let there be some demand for it. That way we also understand better how people use those.

@jeanphilippegg
Copy link
Contributor Author

Yes, I agree! We can still add a user option later.

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

Successfully merging this pull request may close these issues.

None yet

2 participants