bug#34708: alist-get has unclear documentation

[Drew Adams <drew.adams@oracle.com>]

> > > (1) "When using it to set a value, optional argument REMOVE non-nil
> > > means to remove KEY from ALIST if the new value is `eql' to DEFAULT."
> > >
> > > I wonder if there are use cases where the user wants something different
> > > than `eql'?  E.g. `equal' when the associations are strings?  Note that
> > > this is something different than TESTFN which is for comparing keys.
> >
> > I think so, yes.  Why wouldn't we want to allow that?
> 
> To not add one more argument?

An _optional_ arg.  Why wouldn't we want it?

BTW, how does `alist-get' deal with a value that is
a list: `(a 1)' instead of `(a . 1)'?  I guess it
just considers the VALUE to be `(1)'.  If so, is
`eql' really appropriate for most such cases (which
are quite common)?

And even for `(a . (1 2))', aka `(a 1 2)', is `eql'
appropriate?  Since the cdr of a list is more
typically a list, why would we choose `eql' as the
default value-comparison predicate?  To compare
lists the default predicate should be `equal' (or
possibly but not likely `eq'), no?

> If we do that, I guess I would rather allow that the non-nil value of
> REMOVE is allowed to be a function.  A related use case would be to
> allow to delete the association of a key independently from associated
> value.

That'd be OK.  If the second test function would be
only for removal then letting REMOVE be a function
would be fine.  Presumably it would be a predicate,
testing each full entry (the cons that holds both
key and value), not just the value.

> > > (2) The remove feature has a strange corner case.  Normally the
> > > first found association is removed,
> >
> > So "normally" it's really "remove one".
> >
> > Why is this?  What's the point of REMOVE - why is
> > it needed (added to the design) at all?  Is it to
> > provide a way to remove all entries with a given
> > key or only the first such?
> 
> The first.

Then why did (does?) the doc say "if the new value
is `eql' to DEFAULT"?  It sounds like it removes
only the entries with a given key AND a given value.

Anyway, if that's all REMOVE does (removes all
occurrences), and if it can be a predicate, then it
sounds like it just does `cl-delete-if'.

If so, what's an example of why/when someone would
want to use `setf' and `alist-get' to remove entries,
as opposed to just using `cl-delete-if'?

I may be missing something.  I'm not familiar with
the whole bug thread and I'm looking at the existing
(old) doc string.

> > If we want to provide for pretty much everything
> > that one typically does with an alist (without
> > `alist-get') then don't we need to provide for the
> > ability to do any kind of removal - or other
> > operations - on alist elements that have a given key?
> >
> > Should "set" and "remove" operations not (at least
> > be able to) obtain the _full_ sequence (in entry
> > order) of such matching elements, and then perform
> > specific operations on that sequence (such as setting
> > or removing the first one, the Nth one, or all of
> > them)?
> >
> > If we were not trying to allow `alist-get' to be
> > usable as a generalized variable then I suppose
> > we wouldn't need to worry about any of this.
> 
> We tried.  I think the result should be consistent and convenient, but
> we don't need to implement all realizations of all operations for the
> generalized variable.

Then isn't it a bit misleading for the function name
and doc to suggest that this is a general way of using
alists?  There is already some misunderstanding out
there about alists, with some folks thinking that there
should only ever be a single entry with a given key
(which is true of a hash table).  Won't this augment
such confusion?

So far, I guess I don't see the use case for making it
a generalized variable.  It's easy enough to set alist
values, and doing so with `setf' and `alist-get' sounds
more complicated, not less.

For getting, I think I get it: folks apparently don't
want to get the full element and then dig out the value
(cdr) from it.  (Is there more to it than that?)  For
setting and removing, I don't get the advantage, so far.

> One thing I don't find consistent is the case where the alist already
> has multiple occurrences of a key.  E.g.
> 
> (setq my-alist '((a . 1) (b . 2) (a . -1)))
> (setf (alist-get 'a my-alist 1 'remove) 1)
> my-alist ==> ((b . 2) (a . -1))
> 
> (alist-get 'a my-alist 1)
>   ==> -1    (!)
> 
> One would expect 1, of course.

Why?  The doc says that it returns DEFAULT only
if KEY is not found in ALIST.  But entry (a . -1)
finds `a' associated with -1.  What am I missing?

But if you don't find it inconsistent then that's
a problem, because many (most, I expect) uses of
alists do have some multiple occurrences of a key.

In any case, what you show is an example of why I
wouldn't think that `setf' with `alist-get' is
simpler.  It may be less verbose sometimes, but it
doesn't seem as clear.

If, as the doc says, it removes only the entries
with a given key AND a given value, then isn't this:

(setq my-alist  (cl-delete-if
                  (lambda (entry)
                    (and (eql (car entry 'a))
                         (eql (cdr entry 1))))
                  my-alist))

more straightforward than this:

(setf (alist-get 'a my-alist 1 'remove) 1)?

Or if, as I think you're saying, it removes all the
entries with a given key, regardless of the values,
then just this:

(setq my-alist  (cl-delete-if
                  (lambda (entry) (eql (car entry 'a)))
                  my-alist))

I find the `setf' with `remove' and double 1's to be
confusing.  It looks like it removes all entries for
key `a' that have value 1, and then it _creates_ an
entry (a . 1).  I know that it doesn't do that, but
to me that's what it looks like it's saying.

If there really is a good use case for `alist-get'
to be a generalized variable, and for that to let
you remove entries and not just set/create entries,
then it seems like a better syntax could be found.

FWIW, to me the whole remove thing seems to fly in
the face of what `alist-get' and `setf' are about.
With REMOVE `setf' is NOT setting an alist element.
Instead, it's changing the alist structure - it's
not acting on elements of the list.

`alist-get' specifies an alist entry (a single one,
BTW).  `setf' of `alist-get' should set/create an
alist entry (a single one, BTW).  Otherwise, we're
abusing the intention of one or both of these
"functions".  No?

> > It would be good to see a statement/spec of what
> > `alist-get' is trying to accomplish, especially
> > wrt setting, testing (diff predicates), and
> > removing.
> 
> Yes, this is what my patch will try to accomplish.

Great.  Thx.