Org Refile RFLOC and Struct Type

Org Refile RFLOC and Struct Type

[Kevin Foley]

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

Re: Org Refile RFLOC and Struct Type

[Kyle Meyer]

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.

Re: [PATCH] Org Refile Document RFLOC (was: Org Refile RFLOC and Struct Type)

[Kevin Foley]

[-- 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

Re: [PATCH] Org Refile Document RFLOC (was: Org Refile RFLOC and Struct Type)

[Kyle Meyer]

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.