Re: [PATCH] org-attach.el: ID to path functions may return nil

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

>> AFAIU, your version of `org-attach-dir-from-id' may sometimes return
>> nil, which is neither documented
>
> "otherwise independently of TRY-ALL
> value return the first non nil value."
>
> indirectly assumes nil. It may be made more prominent.

I feel like this makes the docstring confusing.

Note that `org-attach-id-to-path-function-list':

    List of functions parsing an ID string into a folder-path.
    The first function in this list defines the preferred function
    which will be used when creating new attachment folders.  All
    functions of this list will be tried when looking for existing
    attachment folders based on ID.

The docstring implies that only the first function will be used to
create new attachment folders. With your patch, it will no longer be the
case and may surprise users.

You at least need to update the docstring.

> If TRY-ALL is non-nil, try all id-to-path functions in
> `org-attach-id-to-path-function-list' and return the first path
> that exist in the filesystem, otherwise independently of TRY-ALL
> value return the first non nil value.

This is a very long sentence and I cannot clearly parse which sub-clause
is referenced by "otherwise".

>> nor expected by the callers.
>
> I see 2 direct callers and I am not aware of any problem with them.
>
>> In particular, `org-attach-dir-get-create' will throw unhelpful "No
>> attachment directory is associated with the current node" error.
>
> * H short
> :PROPERTIES:
> :ID:       ec
> :END:
>
> if: No attachment directory is associated with the current node, adjust 
> ‘org-attach-id-to-path-function-list’
>
> I do not think this message is unhelpful.

With your patch, such message will be displayed even when
`org-attach-preferred-new-method' is set to something other than 'id.
And the message will be wrong then.

> +(defun org-attach-id-fallback-folder-format (id)
> +  "May be added last to `org-attach-id-path-function-list'.

This is not a proper first line in a function docstring. First line must
describe what the function does.

-- 
Ihor Radchenko // yantar92,
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>