I was recently working with `org-refile` and wanted to use a custom
target. There is the `rfloc` argument which is documented as:
> RFLOC can be a refile location obtained in a different way.
There's no documentation as to how `rfloc` should be structured. To
figure that out one has to read through the code which is made even more
difficult by the fact that the same argument is called `refloc` in
`org-refile--get-location`.
I plan to submit a patch to address this, however I wanted feedback on
whether using a struct type (cl-defstruct) could be an improvement
here before trying to implement it.
It seems using a defined structure would make both the documentation and
code more clear, however I rarely (if ever) have seen structures used in
the elisp code I'm familiar with. Is there a downside to using struct
types that would make it a poor choice in this case?
Kevin
Kevin Foley writes: > I was recently working with `org-refile` and wanted to use a custom > target. There is the `rfloc` argument which is documented as: > >> RFLOC can be a refile location obtained in a different way. > > There's no documentation as to how `rfloc` should be structured. To > figure that out one has to read through the code which is made even more > difficult by the fact that the same argument is called `refloc` in > `org-refile--get-location`. True, this should be documented. > I plan to submit a patch to address this, however I wanted feedback on > whether using a struct type (cl-defstruct) could be an improvement > here before trying to implement it. > > It seems using a defined structure would make both the documentation and > code more clear, however I rarely (if ever) have seen structures used in > the elisp code I'm familiar with. Is there a downside to using struct > types that would make it a poor choice in this case? I imagine tastes vary on whether using cl-defstruct here is an overkill. (To my eyes, it is.) More importantly, though, I think changing it now means we'd also need a compatibility layer, which doesn't seem worth the trouble. Thanks for noticing and for working on a patch.
[-- Attachment #1: Type: text/plain, Size: 993 bytes --] Kyle Meyer <kyle@kyleam.com> writes: > I imagine tastes vary on whether using cl-defstruct here is an overkill. > (To my eyes, it is.) More importantly, though, I think changing it now > means we'd also need a compatibility layer, which doesn't seem worth the > trouble. I tried implementing it and realized: 1. It have issues with backwards compatibility (like you mentioned). There are some workarounds but they add complexity 2. It can make other things complicated. For example using `assoc' to look things up no longer works so a lot needs to be updated. Personally I think it's worth it as it makes things much clearer but I understand why others may not feel that way and it's not something I feel strongly enough about to push for. > Thanks for noticing and for working on a patch. My pleasure, I've attached a patch to this email. I put something together but I wasn't sure how it should be styled/formatted so if anyone has any suggestions I'd be happy to update it. Kevin [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-org-refile.el-org-refile-Add-description-of-RFLOC-to.patch --] [-- Type: text/x-patch, Size: 1247 bytes --] From 87af8fc4a08c8b4b2c9c508d0bad0565c0d10429 Mon Sep 17 00:00:00 2001 From: "Kevin J. Foley" <kevin@kevinjfoley.me> Date: Sun, 18 Oct 2020 21:30:52 -0400 Subject: [PATCH] org-refile.el (org-refile) Add description of RFLOC to docstring --- lisp/org-refile.el | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lisp/org-refile.el b/lisp/org-refile.el index 7eb0a9643..4d56f2e9a 100644 --- a/lisp/org-refile.el +++ b/lisp/org-refile.el @@ -414,7 +414,16 @@ (defun org-refile (&optional arg default-buffer rfloc msg) Beware that keeping refiled entries may result in duplicated ID properties. -RFLOC can be a refile location obtained in a different way. +RFLOC can be a refile location obtained in a different way. It +should be a list with the following 4 elements: + +1. Name - an identifier for the refile location, typically the +headline text. +2. File - the file the refile location is in +3. nil - Used for generating refile location candidates, not +needed when passing RFLOC +4. Position- the position in the specified file of the +headline to refile under MSG is a string to replace \"Refile\" in the default prompt with another verb. E.g. `org-copy' sets this parameter to \"Copy\". -- 2.28.0
Kevin Foley writes: > My pleasure, I've attached a patch to this email. I put something > together but I wasn't sure how it should be styled/formatted so if > anyone has any suggestions I'd be happy to update it. Thanks. Looks good. Applied (2d1e2606e), tweaking the commit message to use a colon separator in the subject and to include a changelog entry. > -RFLOC can be a refile location obtained in a different way. > +RFLOC can be a refile location obtained in a different way. It > +should be a list with the following 4 elements: > + > +1. Name - an identifier for the refile location, typically the > +headline text. > +2. File - the file the refile location is in > +3. nil - Used for generating refile location candidates, not > +needed when passing RFLOC > +4. Position- the position in the specified file of the > +headline to refile under For consistency across these entries, I've dropped the period from the first entry, downcased "Used", and added a space before the hyphen in 4.