bug#35206: [PATCH] Misleading `list-get' argument description

bug#35206: [PATCH] Misleading `list-get' argument description

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 320 bytes --]

The doc string for `list-get' says

  Use TESTFN to lookup in the alist if non-nil.  Otherwise, use `assq'.

which is misleading since it's an equality predicate, not a look-up function, and the default is `eq', not `assq'.
How about changing it to

  Equality is defined by TESTFN or by `eq' if nil or omitted.

[-- Attachment #2: 0001-Clarify-the-TESTFN-argument-to-alist-get.patch --]
[-- Type: application/octet-stream, Size: 1088 bytes --]

From 0bdfa84e6c55ff97286818b42ed3e6f035d47151 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Tue, 9 Apr 2019 12:27:30 +0200
Subject: [PATCH] Clarify the TESTFN argument to `alist-get'

* lisp/subr.el (alist-get): Clarify the meaning of the TESTFN argument.
It's an equality predicate, not a look-up function.
---
 lisp/subr.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/subr.el b/lisp/subr.el
index 45b3916196..5eb6a53f78 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -781,7 +781,7 @@ Elements of ALIST that are not conses are ignored."
 (defun alist-get (key alist &optional default remove testfn)
   "Return the value associated with KEY in ALIST.
 If KEY is not found in ALIST, return DEFAULT.
-Use TESTFN to lookup in the alist if non-nil.  Otherwise, use `assq'.
+Equality is defined by TESTFN or by `eq' if nil or omitted.
 
 You can use `alist-get' in PLACE expressions.  This will modify
 an existing association (more precisely, the first one if
-- 
2.20.1 (Apple Git-117)

bug#35206: [PATCH] Misleading `list-get' argument description

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Tue, 9 Apr 2019 12:34:09 +0200
> 
> The doc string for `list-get' says
> 
>   Use TESTFN to lookup in the alist if non-nil.  Otherwise, use `assq'.
> 
> which is misleading since it's an equality predicate, not a look-up function, and the default is `eq', not `assq'.
> How about changing it to
> 
>   Equality is defined by TESTFN or by `eq' if nil or omitted.

IMO, this has a problem similar to the original text: it has nothing
to "connect" it to the main part of the doc string, which is this:

  Return the value associated with KEY in ALIST.

Neither "look up" nor "equality" is directly and obviously related to
"associated with".  E.g., you probably like "equality" better because
your mental model of "associated with" is a test for equality;
however, someone else might prefer the current text because they think
of "looking up" in the same situation.  But the text doesn't make the
relation explicit, and that is IMO its main problem.

Can you think of a change that would resolve this problem?

Thanks.

bug#35206: [PATCH] Misleading `list-get' argument description

[Mattias Engdegård]

9 apr. 2019 kl. 13.00 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> Neither "look up" nor "equality" is directly and obviously related to
> "associated with".  E.g., you probably like "equality" better because
> your mental model of "associated with" is a test for equality;
> however, someone else might prefer the current text because they think
> of "looking up" in the same situation.  But the text doesn't make the
> relation explicit, and that is IMO its main problem.

Good point. What about:

Comparison of KEY against the car of each ALIST element
is made using TESTFN, or `eq' if nil or omitted.

bug#35206: [PATCH] Misleading `list-get' argument description

[Mattias Engdegård]

> 
> Comparison of KEY against the car of each ALIST element
> is made using TESTFN, or `eq' if nil or omitted.

If no knowledge whatsoever of alists can be assumed on the part of the reader, perhaps this would be better:

   "Return the value associated with KEY in ALIST.
+The value is the cdr of the first element in ALIST whose car is equal to KEY.
 If KEY is not found in ALIST, return DEFAULT.
-Use TESTFN to lookup in the alist if non-nil.  Otherwise, use `assq'.
+Equality is defined by TESTFN or by `eq' if nil or omitted.

bug#35206: [PATCH] Misleading `list-get' argument description

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Tue, 9 Apr 2019 13:45:34 +0200
> Cc: 35206@debbugs.gnu.org
> 
> > 
> > Comparison of KEY against the car of each ALIST element
> > is made using TESTFN, or `eq' if nil or omitted.
> 
> If no knowledge whatsoever of alists can be assumed on the part of the reader, perhaps this would be better:
> 
>    "Return the value associated with KEY in ALIST.
> +The value is the cdr of the first element in ALIST whose car is equal to KEY.
>  If KEY is not found in ALIST, return DEFAULT.
> -Use TESTFN to lookup in the alist if non-nil.  Otherwise, use `assq'.
> +Equality is defined by TESTFN or by `eq' if nil or omitted.

This is much better, IMO.  It is better than your previous proposal,
because the text is simpler and thus more clear.

However, a slight rewording would improve it even more:

  Find an element of ALIST whose `car' equals KEY and return its `cdr'.
  ...
  Equality with KEY is tested by TESTFN, defaulting to `eq'.

IMO, this isn't about assuming knowledge, this is about being as
explicit as reasonably possible about what the function does.
(Strictly speaking, both your suggestion and mine still assume some
knowledge about alists, because we never explain what is an alist, nor
what is an "element" of an alist.)

Thanks.

bug#35206: [PATCH] Misleading `list-get' argument description

[Mattias Engdegård]

9 apr. 2019 kl. 16.41 skrev Eli Zaretskii <eliz@gnu.org>:
> 
>  Find an element of ALIST whose `car' equals KEY and return its `cdr'.
>  ...
>  Equality with KEY is tested by TESTFN, defaulting to `eq'.

Thank you, I pushed that change (except that I used "the first element" instead of "an element" for precision).

> IMO, this isn't about assuming knowledge, this is about being as
> explicit as reasonably possible about what the function does.
> (Strictly speaking, both your suggestion and mine still assume some
> knowledge about alists, because we never explain what is an alist, nor
> what is an "element" of an alist.)

That's all right -- I was mainly concerned with the quite misleading TESTFN description, and looked at the docs of similar functions (assq, assoc).