[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#34708: alist-get has unclear documentation
From: |
Drew Adams |
Subject: |
bug#34708: alist-get has unclear documentation |
Date: |
Mon, 11 Mar 2019 10:48:16 -0700 (PDT) |
> > > (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.
- bug#34708: alist-get has unclear documentation, (continued)
- bug#34708: alist-get has unclear documentation, Michael Heerdegen, 2019/03/04
- bug#34708: alist-get has unclear documentation, Eric Abrahamsen, 2019/03/04
- bug#34708: alist-get has unclear documentation, Michael Heerdegen, 2019/03/04
- bug#34708: alist-get has unclear documentation, Eric Abrahamsen, 2019/03/04
- bug#34708: alist-get has unclear documentation, Michael Heerdegen, 2019/03/05
- bug#34708: alist-get has unclear documentation, Eric Abrahamsen, 2019/03/05
- bug#34708: alist-get has unclear documentation, Drew Adams, 2019/03/05
- bug#34708: alist-get has unclear documentation, Michael Heerdegen, 2019/03/11
- bug#34708: alist-get has unclear documentation, Drew Adams, 2019/03/11
- bug#34708: alist-get has unclear documentation, Michael Heerdegen, 2019/03/11
- bug#34708: alist-get has unclear documentation,
Drew Adams <=
- bug#34708: alist-get has unclear documentation, Michael Heerdegen, 2019/03/12
- bug#34708: alist-get has unclear documentation, Drew Adams, 2019/03/12
- bug#34708: alist-get has unclear documentation, Michael Heerdegen, 2019/03/12
- bug#34708: alist-get has unclear documentation, Drew Adams, 2019/03/12
- bug#34708: alist-get has unclear documentation, Michael Heerdegen, 2019/03/12
- bug#34708: alist-get has unclear documentation, Michael Heerdegen, 2019/03/12
- bug#34708: alist-get has unclear documentation, Drew Adams, 2019/03/12
- bug#34708: alist-get has unclear documentation, Michael Heerdegen, 2019/03/12
- bug#34708: alist-get has unclear documentation, Drew Adams, 2019/03/12
- bug#34708: alist-get has unclear documentation, Michael Heerdegen, 2019/03/12