Quick review
Closed this issue · 8 comments
Hi,
thank you for this package! I quickly looked over the code and have a few comments and improvement proposals.
- The variable
consult-org-roam-no-preview-functions
is unnecessary. You can useconsult-customize
to selectively disable preview per command.
consult-org-roam/consult-org-roam.el
Line 43 in 3348f97
consult-org-roam--preview-functions
can be removed.
consult-org-roam/consult-org-roam.el
Line 239 in 3348f97
- Instead of
add-to-list
I recommend to usepush
followed by a subsequentdelete-dups
on the final list. It will be more efficient.
consult-org-roam/consult-org-roam.el
Line 107 in 3348f97
consult-org-roam--temporary-nodes
looks likeconsult--temporary-files
. Better reuseconsult--temporary-files
such that you avoid the code duplication and such that you profit from future improvements to that function.
consult-org-roam/consult-org-roam.el
Line 156 in 3348f97
- You can pass
org-roam-ref-annotation-function
directly. There is no need for the lambda.
consult-org-roam/consult-org-roam.el
Line 233 in 3348f97
- Use
define-minor-mode
.
consult-org-roam/consult-org-roam.el
Line 250 in 3348f97
- If you use
define-minor-mode
, themessage
calls are unnecessary.
consult-org-roam/consult-org-roam.el
Line 266 in 3348f97
Hi @minad,
thank you very much for looking at the code. Your interest in this package and the input on this is much appreciated!
PR #4 implements almost all of you proposals for improvement. However, I refrained from reusing consult--temporary-files
for consult-org-roam--temporary-nodes
for two reasons: First, I need to derive the filename via org-roam-node-file node
for each node, and, secondly, I intend to improve this function to open the actual org-roam-node and not just the housing file. Is this okay for you? I hope #4 is fine, do you have any further recommendations?
Thanks again and best regards,
jgru
Of course not using the temporary file function is perfectly okay if it is sufficiently different. But I don't get what the difference is between a node and a file. From my understanding I thought these are just the same? Or at least you have to open the file first and then jump to the headline. In this case you should still reuse consult--temporary-files for the file opening.
Of course not using the temporary file function is perfectly okay if it is sufficiently different. But I don't get what the difference is between a node and a file. From my understanding I thought these are just the same? Or at least you have to open the file first and then jump to the headline. In this case you should still reuse consult--temporary-files for the file opening.
Actually, a node is represented by an ID (of a headline) within an org-file. In code a node is a struct created by cl-defstruct
.
I was wondering whether there is a better approach, but I didn't find a way to pass a filepath instead of a node-struct to consult-org-roam--temporary-nodes
. Can you give me a hint, how to pass filenames (at best with a precise position inside it) instead of nodes to consult--temporary-files
?
It should be possible to retrieve the node id and file path from the org roam node struct, or not?
It should be possible to retrieve the node id and file path from the org roam node struct, or not?
Sure, I did this in my "sodomized" version of consult--temporary-files
, but now I moved this functionality into consult-org-roam--node-preview
. (See 6885fb1)
I think this resolves all of the valuable remarks brought up by you. However, in the future I will have to figure out how to pass the relevant position of the headline to the open-function. (If you have any ideas on this, please let me know.
Thanks again for helping out and best regards,
jgru
Very good. Note that I am currently improving the file preview (minad/consult@3b0d745), so it makes a lot of sense to not duplicate consult--temporary-files
.
However, in the future I will have to figure out how to pass the relevant position of the headline to the open-function. (If you have any ideas on this, please let me know.
You can use the buffer returned by consult--temporary-files
and then move the point inside that buffer. The point is made available by the org-roam-node struct it seems. Or you use the title string and jump there.
Very good. Note that I am currently improving the file preview (minad/consult@3b0d745), so it makes a lot of sense to not duplicate
consult--temporary-files
.
Good to hear. I will keep an eye on this improvement. :)
You can use the buffer returned by
consult--temporary-files
and then move the point inside that buffer. The point is made available by the org-roam-node struct it seems. Or you use the title string and jump there.
Okay, then I will just have to modify the preview function. Thanks for the hint on this.