(feat): org-roam-doctor: fix broken links #570
Conversation
|
I like the idea, and I like making this into its own library. An earlier idea I had for implementing this would have been to run |
|
oh... that saved me a lot of trouble |
org-roam-doctor.el
Outdated
| (unless (org-in-regexp org-link-bracket-re 1) | ||
| (user-error "No link at point")) | ||
| (save-excursion | ||
| (delete-region (match-beginning 0) (match-end 0)) |
There was a problem hiding this comment.
I think this should be done in a temp-buffer. Otherwise, if you C-g in a middle of the org-roam-insert meant to fix it, the link is removed from the current buffer.
A better option might be to catch errors/interrupts with a condition-case which would revert the buffer to its pre-doctor state.
There was a problem hiding this comment.
org-roam-insert doesn't really work in a temp-buffer though.
There was a problem hiding this comment.
Then the second option I've mentioned might be best.
There was a problem hiding this comment.
done... seems a bit inefficient though.
org-roam-doctor.el
Outdated
| (match-string-no-properties 2) | ||
| (org-link-unescape (match-string-no-properties 1))))) | ||
| (delete-region (match-beginning 0) (match-end 0)) | ||
| (org-roam-insert nil nil label)))) |
There was a problem hiding this comment.
If we want to keep the label, chances are we might be interested in pre-selecting any candidate that starts with that label. For instance, if I have a broken link with the label Metaphor, fixing it should pre-select Metaphor in my list. This could be done by adding a PRE-SELECT argument to org-roam-insert which, when set, would attempt to place the matched candidate at the top of the completions. Sorting also has the benefit of being completion-engine-agnostic.
There was a problem hiding this comment.
can this be done with regular completing-read? Not familiar with how to do this.
| (unless (memq buf existing-buffers) | ||
| (save-buffer buf) | ||
| (kill-buffer buf))))) | ||
| (message "Linting completed.")) |
There was a problem hiding this comment.
Maybe we could print stats here by incrementing variables for fixed and skipped links.
org-roam-doctor.el
Outdated
| (p (point))) | ||
| (condition-case nil | ||
| (save-excursion | ||
| (delete-region (match-beginning 0) (match-end 0)) |
There was a problem hiding this comment.
| (delete-region (match-beginning 0) (match-end 0)) | |
| (replace-match "")) |
org-roam-doctor.el
Outdated
| (defun org-roam-doctor--replace-link () | ||
| "Replace the current link with a new link." | ||
| (unless (org-in-regexp org-link-bracket-re 1) | ||
| (user-error "No link at point")) | ||
| (let ((orig (buffer-string)) | ||
| (p (point))) | ||
| (condition-case nil | ||
| (save-excursion | ||
| (delete-region (match-beginning 0) (match-end 0)) |
There was a problem hiding this comment.
Do we want to save-match-data here?
org-roam-doctor.el
Outdated
| (defun org-roam-doctor-broken-links (ast) | ||
| "Checker for detecting broken links. | ||
| AST is the org-element parse tree." | ||
| (org-element-map ast 'link | ||
| (lambda (l) | ||
| (when (equal "file" (org-element-property :type l)) | ||
| (let ((file (org-element-property :path l))) | ||
| (and (not (file-remote-p file)) | ||
| (not (file-exists-p file)) | ||
| (list (org-element-property :begin l) | ||
| (format (if (org-element-lineage l '(link)) | ||
| "Link to non-existent image file \"%s\"\ | ||
| in link description" | ||
| "Link to non-existent local file \"%s\"") | ||
| file)))))))) |
There was a problem hiding this comment.
| (defun org-roam-doctor-broken-links (ast) | |
| "Checker for detecting broken links. | |
| AST is the org-element parse tree." | |
| (org-element-map ast 'link | |
| (lambda (l) | |
| (when (equal "file" (org-element-property :type l)) | |
| (let ((file (org-element-property :path l))) | |
| (and (not (file-remote-p file)) | |
| (not (file-exists-p file)) | |
| (list (org-element-property :begin l) | |
| (format (if (org-element-lineage l '(link)) | |
| "Link to non-existent image file \"%s\"\ | |
| in link description" | |
| "Link to non-existent local file \"%s\"") | |
| file)))))))) | |
| (defun org-roam-doctor-broken-links (ast) | |
| "Checker for detecting broken links. | |
| AST is the org-element parse tree." | |
| (org-element-map ast 'link | |
| (lambda (l) | |
| (when (equal "file" (org-element-property :type l)) | |
| (let ((file (org-element-property :path l))) | |
| (and (not (or (file-exists-p file) | |
| (file-remote-p file))) | |
| `(,(org-element-property :begin l) | |
| ,(format (if (org-element-lineage l '(link)) | |
| "Link to non-existent image file \"%s\"\ | |
| in link description" | |
| "Link to non-existent local file \"%s\"") | |
| file)))))))) |
I grouped the file predicates in an or and reversed the order. This should be more efficient, as the or will short-circuit on the first nil case. I believe that will be file-exists-p more often than file-remote-p. It's probably a micro-optimization, but it should be logically equivalent.
org-roam-doctor.el
Outdated
| (defun org-roam-doctor (&optional this-buffer) | ||
| "Perform a check on Org-roam files to ensure cleanliness. | ||
| If THIS-BUFFER, run the check only for the current buffer." | ||
| (interactive "P") | ||
| (let ((existing-buffers (org-roam--get-roam-buffers)) | ||
| files) | ||
| (if (not this-buffer) | ||
| (setq files (org-roam--list-all-files)) | ||
| (unless (org-roam--org-roam-file-p) | ||
| (user-error "Not in an org-roam file")) | ||
| (setq files (list (buffer-file-name)))) | ||
| (save-window-excursion | ||
| (dolist (f files) | ||
| (let ((buf (find-file-noselect f))) | ||
| (with-current-buffer buf | ||
| (org-roam-doctor--check buf org-roam-doctor--checkers)) | ||
| (unless (memq buf existing-buffers) | ||
| (save-buffer buf) | ||
| (kill-buffer buf)))))) | ||
| (message "Linting completed.")) |
There was a problem hiding this comment.
Perhaps we could introduce a customizable variable, org-roam-doctor-files which defaults to (org-roam-list-all-files). org-roam-doctor would run its checks against the files in this list. Users/commands could then customize that list to check a subset.
e.g.
(let ((org-roam-doctor-files '("example.org" "etc...")))
(org-roam-doctor))This way it's not an all or single file proposition.
similar to org-agenda-files.
org-roam.el
Outdated
| (defun org-roam-insert (&optional prefix filter-fn description) | ||
| "Find an Org-roam file, and insert a relative org link to it at point. | ||
| If PREFIX, downcase the title before insertion. | ||
| FILTER-FN is the name of a function to apply on the candidates | ||
| which takes as its argument an alist of path-completions. See | ||
| which takes as its argument an alist of path-completions. | ||
| If DESCRIPTION is provided, use this as the link label. See |
There was a problem hiding this comment.
If prefix is meant to downcase the title before insertion, can we give it a descriptive name? e.g. downcase or lowercase?
org-roam-doctor.el
Outdated
| ;;;###autoload | ||
| (defun org-roam-doctor (&optional this-buffer) | ||
| "Perform a check on Org-roam files to ensure cleanliness. | ||
| If THIS-BUFFER, run the check only for the current buffer." | ||
| (interactive "P") | ||
| (let (files) | ||
| (if (not this-buffer) | ||
| (setq files (org-roam--list-all-files)) | ||
| (unless (org-roam--org-roam-file-p) | ||
| (user-error "Not in an org-roam file")) | ||
| (setq files (list (buffer-file-name)))) | ||
| (org-roam-doctor files org-roam-doctor--checkers))) |
There was a problem hiding this comment.
| ;;;###autoload | |
| (defun org-roam-doctor (&optional this-buffer) | |
| "Perform a check on Org-roam files to ensure cleanliness. | |
| If THIS-BUFFER, run the check only for the current buffer." | |
| (interactive "P") | |
| (let (files) | |
| (if (not this-buffer) | |
| (setq files (org-roam--list-all-files)) | |
| (unless (org-roam--org-roam-file-p) | |
| (user-error "Not in an org-roam file")) | |
| (setq files (list (buffer-file-name)))) | |
| (org-roam-doctor files org-roam-doctor--checkers))) | |
| ;;;###autoload | |
| (defun org-roam-doctor (&optional all) | |
| "Perform a check on the current Org-roam file to ensure cleanliness. | |
| If ALL is non-nil, run the check only for all org-roam files." | |
| (interactive "P") | |
| (let (files (if all | |
| (org-roam--list-all-files) | |
| (unless (org-roam--org-roam-file-p) | |
| (user-error "Not in an org-roam file")) | |
| `(,(buffer-file-name)))) | |
| (org-roam-doctor-start files org-roam-doctor--checkers))) |
I think the last line is intended to call org-roam-doctor-start, correct?
Do you think reversing the semantics of org-roam-doctor's argument would be better UX?
The larger one's set of files gets, the longer this command will block Emacs.
Do we want to present that as the typical use-case, or as an optional use-case?
Ideally we could provide the best of both worlds and implement the whole thing asynchronously, but that comes with challenges as well (e.g. what happens if you're editing the files as they're being checked...).
There was a problem hiding this comment.
Do you think reversing the semantics of org-roam-doctor's argument would be better UX?
Hmm perhaps so.
The larger one's set of files gets, the longer this command will block Emacs.
I want to add a linted column to the files table, so org-roam-doctor can mark a file as linted once it does a pass-through. Modifying a file would then reset the value of that column to nil. That should make linting fast enough to run, to org-roam-db-build-cache.
`org-roam-doctor` provides a diagnostic tool for checking an Org-roam file for things that are broken. Currently implemented is a check for broken links, and methods to fix them. The checker is designed to be extensible.
|
Is it possible to |
|
Renaming the file in Emacs will already update all the links. Try to do it in Dired. |
|
@zaeph that's definitely new to me! Would you mind giving a bit more pointers? |
|
Well, there isn't any. When |
|
That's surprising.. say I renamed |
Motivation for this change
This library provides
org-roam-doctor, a utility for diagnosing and fixingOrg-roam files. Running
org-roam-doctorlaunches a list of checks definedby
org-roam-doctor--checkers. Every checker is an instance oforg-roam-doctor-checker.Each checker is given the Org parse tree (AST), and is expected to return a
list of errors. The checker can also provide "actions" for auto-fixing errors
(see `org-roam-doctor--remove-link' for an example).
The UX experience is inspired by both org-lint and checkdoc, and their code
is heavily referenced.
Closes #222