all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Drew Adams <drew.adams@oracle.com>
To: Michael Heerdegen <michael_heerdegen@web.de>
Cc: Eric Abrahamsen <eric@ericabrahamsen.net>, 34708@debbugs.gnu.org
Subject: bug#34708: alist-get has unclear documentation
Date: Mon, 11 Mar 2019 10:48:16 -0700 (PDT)	[thread overview]
Message-ID: <6e0c5a88-34db-4957-9cc5-98a14ae64f9f@default> (raw)
In-Reply-To: <87wol5l6xk.fsf@web.de>

> > > (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.





  reply	other threads:[~2019-03-11 17:48 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-02  4:50 bug#34708: alist-get has unclear documentation Miguel V. S. Frasson
2019-03-02  9:25 ` Michael Heerdegen
2019-03-02 15:40   ` Miguel V. S. Frasson
2019-03-02 18:10     ` Michael Heerdegen
2019-03-02 19:06       ` Eric Abrahamsen
2019-03-03  0:15         ` Phil Sainty
2019-03-03 12:50           ` Michael Heerdegen
2019-03-19  1:35             ` Michael Heerdegen
2019-03-02 19:51       ` Miguel V. S. Frasson
2019-03-02 20:32         ` Eric Abrahamsen
2019-03-03 11:32       ` Miguel V. S. Frasson
2019-03-03 12:21         ` Michael Heerdegen
2019-03-03 15:51           ` Drew Adams
2019-03-03 16:49             ` Eric Abrahamsen
2019-03-04 16:24             ` Eric Abrahamsen
2019-03-04 16:38               ` Michael Heerdegen
2019-03-04 17:16                 ` Eric Abrahamsen
2019-03-04 18:22                   ` Michael Heerdegen
2019-03-04 22:49                     ` Eric Abrahamsen
2019-03-05 12:35                       ` Michael Heerdegen
2019-03-05 22:50                         ` Eric Abrahamsen
2019-03-06  0:16                           ` Drew Adams
2019-03-11 13:39                             ` Michael Heerdegen
2019-03-11 14:52                               ` Drew Adams
2019-03-11 16:19                                 ` Michael Heerdegen
2019-03-11 17:48                                   ` Drew Adams [this message]
2019-03-12 13:04                                     ` Michael Heerdegen
2019-03-12 14:48                                       ` Drew Adams
2019-03-12 16:08                                         ` Michael Heerdegen
2019-03-12 16:48                                           ` Drew Adams
2019-03-12 17:45                                             ` Michael Heerdegen
2019-03-12 13:12                                 ` Michael Heerdegen
2019-03-12 14:53                                   ` Drew Adams
2019-03-12 15:38                                     ` Michael Heerdegen
2019-03-12 16:18                                       ` Drew Adams
2019-03-12 17:55                                         ` Michael Heerdegen
2019-03-15 15:54                                           ` Michael Heerdegen
2019-03-15 18:48                                             ` Eric Abrahamsen
2019-03-27 22:31                                               ` Michael Heerdegen
2019-04-19  1:33                                                 ` Michael Heerdegen
2019-04-19  2:24 ` bug#34708: Thanks Miguel V. S. Frasson
2019-04-19  4:18   ` Michael Heerdegen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6e0c5a88-34db-4957-9cc5-98a14ae64f9f@default \
    --to=drew.adams@oracle.com \
    --cc=34708@debbugs.gnu.org \
    --cc=eric@ericabrahamsen.net \
    --cc=michael_heerdegen@web.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.