Re: [PATCH v4] [BUG] org-attach-id-ts-folder-format fails on customized IDs [9.6 (9.6-??-2e9999783)]

[Ihor Radchenko <yantar92@gmail.com>]

Max Nikulin <manikulin@gmail.com> writes:

>> The new version of the patch allows all but first function return nil.
>> See the attached.
>> 
>> Let me know if there are any other objections.
>
> Ihor, sorry that I left your earlier questions with no response. I hope, 
> the attached diff may illustrate my ideas better. I have not tried to 
> run it though, so it can be full of stupid mistakes. I suggest to ignore 
> nil values completely, but maybe I missed some use case.

AFAIU, your version of `org-attach-dir-from-id' may sometimes return
nil, which is neither documented nor expected by the callers.

In particular, `org-attach-dir-get-create' will throw unhelpful "No
attachment directory is associated with the current node" error.

`org-attach-dir' appears to handle nil return value fine.

I will be happy to merge a proper patch upon addressing possible
pitfalls. I do not feel strongly about my variant vs. yours.

> Actually I suspect the if the `org-attach-dir-from-id' function returned 
> both first and existing directories, the code of callers would be
> clearer.

`org-attach-dir-from-id' is a public function, and we cannot change its
return signature drastically without breaking backwards compatibility.
If we do want to change the return value, the current function name
should be obsoleted and an alternative function should be introduced.

I do not feel like the benefits are strong enough to justify such a
drastic measure. `org-attach-dir-from-id' is only used in two places in
Org code - I see no clear benefit of returning multiple paths.

-- 
Ihor Radchenko,
Org mode contributor,
Learn more about Org mode at https://orgmode.org/.
Support Org development at https://liberapay.com/org-mode,
or support my work at https://liberapay.com/yantar92