Re: Feedback on changes to org-id

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Aaron Ecay <aaronecay@gmail.com> writes:

> It’s an occasional project of mine to try to improve and refactor
> aspects of org’s code.

That's a very good idea, thanks.

BTW, please make sure that deprecation warnings all go into the same
location, namely "org-compat.el", instead of scattering them all over
the place.

> I’ve been going through org-id recently.  The
> following issues came up that I would appreciate feedback on:
>
> 1. org-id is not loaded by default; it is supposed to be selected by the
>    user (see footnote 2 of (info "(org) Handling links")).  Yet, other
>    org libraries rely on it, without requiring it (e.g. ox-icalendar).
>    Also, at least one library (org-bibtex) reimplements bits of org-id’s
>    internals to avoid requiring it explicitly.  Is there a good reason
>    not to make org-id a core library, that is to require it
>    unconditionally from org.el?

The reason is loading speed. That is also why "ox.el" is not loaded by
default. Instead, loading mechanism relies on autoloads, i.e., you don't
have to explicitly load it, it will be so as soon as you call one of its
public functions.

I think "org-bibtex" should simply assume "org-id" is loaded and call
whatever function is needed. There is no reason to avoid requiring it
explicitly.

At least, that's the idea. OTOH, I'm not opposed to requiring it
explicitly.

> 2. I would like to deprecate the org-id-method variable.  This allows
>    choosing different methods of generating random IDs.  But one method
>    is as good as another (they are random*...), so we can just always use
>    a single method (powered by the elisp ‘random’ function).  Choosing
>    this allows deprecating several other variables and functions, along
>    with a soft dependency on the external uuidgen binary.  Any
>    objections to this course of action?

OK.

> 3. I would like to change the API of the org-id-get function.  The
>    current signature is (&optional pom create prefix).  POM
>    (i.e. position or marker) is a useless argument, because in the
>    (relatively uncommon) case that callers are interested in a location
>    other than (point) they can wrap the call in (org-with-point-at
>    ...).

I sympathize with the idea, but the POM argument is consistent with the
property API (e.g., org-entry-get'). So, it seems awkward to change one
part without touching the others. I suggest to keep it for the time
being.

>    PREFIX is similarly useless (and in fact unused in org’s code base)
>    because a caller could let-bind org-id-prefix around the call.  The
>    new signature would be (&optional create reset), which are both
>    boolean arguments.  The question arises of how to make this change.
>    Options I see:
>    a. Hard breakage; code using the old calling convention will break.
>    b. Introduce a new function under a new name, deprecate the old name
>    c. Try to detect which calling convention is in use.
>    Options (a) and (b) have drawbacks.  I would like to implement
>    (c) by requiring the create and reset arguments, if given, to have
>    values 'create and 'reset respectively.  The old and new calling
>    conventions have identical semantics when both arguments are nil, so
>    that case is not a problem.  With the new code, any other value for
>    these arguments (besides nil and a same-named symbol) would indicate
>    use of the old convention, and signal an error.  Comments?

I don't like (c) because it probably means we're not going to let it go
anytime soon. Also, it means that (org-id-get t t) is not valid using
the new signature.

I'd go with (a) after Org 9.0 is released, or (b) right now, e.g., with
a new `org-id' function (which is shorter, BTW).

> 4. A similar issue arises for org-id-find.  I would like it to always
>    return a marker, rather than having an argument switch between a
>    marker and a cons of filename and position.  (There are functions
>    which return the filename or position individually, so returning both
>    as a cons is useless from an API point of view).  There’s no good way
>    to detect the old calling convention, however, so I think I have to
>    introduce a new name.  (My draft patch is written instead with hard
>    breakage, but stability of API is important so I will change
>    that...)

Please don't make that change. A marker is pointless if the file is not
currently associated to any buffer. In that case (file-name . postition)
cons cell is a valuable return value.

Besides, a function always returning a marker is almost certainly a bad
idea. Markers need to be reused, when appropriate, or set to nil.
Otherwise, you leave active markers everywhere, which slows down Emacs.
IOW, requiring explicitly a marker is a way to think about the future of
the marker and leads to better code.

> There are other deprecations and renamings as well, but none of them
> should break third-party code.  The resultant patch shrinks the codebase
> by 60-ish lines and eliminates 3 defcustoms...baby steps.

You can also remove `org-id-track-globally'. "org-id.el" is useless if
it is set to nil anyway, since CUSTOM_ID does a better job in this case.

>A draft patch is attached to this message; I expect to make further
>changes based on feedback I receive, so detailed code review (while
>certainly always appreciated!) can be postponed until the conceptual

> -(defvar org-id-files nil
> -  "List of files that contain IDs.")
> +(defconst org-id-files nil
> +  "Use the function instead.")
> +(make-obsolete-variable 'org-id-files "use the identically-named
> function instead." "Org 9.0")

This should go in org-compat.el. The same goes for other occurrences.

Regards,

-- 
Nicolas Goaziou