all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#37063: 26.2.90; Problems with recent CL support in checkdoc
@ 2019-08-17 11:41 Damien Cassou
  2019-09-30 16:52 ` dick.r.chiang
  2019-10-10  0:22 ` Lars Ingebrigtsen
  0 siblings, 2 replies; 7+ messages in thread
From: Damien Cassou @ 2019-08-17 11:41 UTC (permalink / raw)
  To: 37063; +Cc: Alex Branham, Lars Ingebrigtsen

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

In the context of bug#37034, some initial support for CL functions and
methods was added to checkdoc.  On my code, the changes make checkdoc
trigger warnings on perfectly valid code IMO. For example,

  (cl-defmethod foo ((a list)) "Return A.")

Checkdoc complains that LIST should be described. I disagree with that,
list is a type, not a argument name.

I attach to this bug report a patch introducing test cases to
checkdoc-tests.el. If it's not possible to implement full support for CL
I prefer that checkdoc doesn't complain even if there is a bug in the
docstring rather than checkdoc complains when the docstring is correct.

Best,

-- 
Damien Cassou
http://damiencassou.seasidehosting.st

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-checkdoc-CL-tests.patch --]
[-- Type: text/x-patch, Size: 3192 bytes --]

From d7357884819f8921ff1a27dd3a25bdfd882d1c5a Mon Sep 17 00:00:00 2001
From: Damien Cassou <damien@cassou.me>
Date: Fri, 16 Aug 2019 20:42:26 +0200
Subject: [PATCH] checkdoc CL tests

---
 test/lisp/emacs-lisp/checkdoc-tests.el | 61 ++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/test/lisp/emacs-lisp/checkdoc-tests.el b/test/lisp/emacs-lisp/checkdoc-tests.el
index 1cefc4c366..ee4c1fb5a6 100644
--- a/test/lisp/emacs-lisp/checkdoc-tests.el
+++ b/test/lisp/emacs-lisp/checkdoc-tests.el
@@ -37,6 +37,67 @@
     (insert "(defun foo())")
     (should-error (checkdoc-defun) :type 'user-error)))
 
+(ert-deftest checkdoc-cl-defmethod-ok ()
+  "Checkdoc should be happy with a simple correct cl-defmethod."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (insert "(cl-defmethod foo (a) \"Return A.\")")
+    (checkdoc-defun)))
+
+(ert-deftest checkdoc-cl-defmethod-with-types-ok ()
+  "Checkdoc should be happy with a cl-defmethod using types."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    ;; this method matches if A is the symbol `smthg' and if b is a list:
+    (insert "(cl-defmethod foo ((a (eql smthg)) (b list)) \"Return A+B.\")")
+    (checkdoc-defun)))
+
+(ert-deftest checkdoc-cl-defun-with-key-ok ()
+  "Checkdoc should be happy with a cl-defun using &key."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (insert "(cl-defun foo (&key a (b 27)) \"Return :A+:B.\")")
+    (checkdoc-defun)))
+
+(ert-deftest checkdoc-cl-defun-with-allow-other-keys-ok ()
+  "Checkdoc should be happy with a cl-defun using &allow-other-keys."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (insert "(cl-defun foo (&key a &allow-other-keys) \"Return :A.\")")
+    (checkdoc-defun)))
+
+(ert-deftest checkdoc-cl-defun-with-aux-ok ()
+  "Checkdoc should be happy with a cl-defun using &aux."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (insert "(cl-defun foo (a b &aux (c (+ a b))) \"Return A and B.\")")
+    (checkdoc-defun)))
+
+(ert-deftest checkdoc-cl-defun-with-default-optional-value-ok ()
+  "Checkdoc should be happy with a cl-defun using default values for optional args."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    ;; B is optional and equals 1+a if not provided. HAS-BS is non-nil
+    ;; if B was provided in the call:
+    (insert "(cl-defun foo (a &optional (b (1+ a) has-bs)) \"Return A + B.\")")
+    (checkdoc-defun)))
+
+(ert-deftest checkdoc-cl-defun-with-destructuring-ok ()
+  "Checkdoc should be happy with a cl-defun destructuring its arguments."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (insert "(cl-defun foo ((a b &optional c) d) \"Return A+B+C+D.\")")
+    (checkdoc-defun)))
+
+(ert-deftest checkdoc-cl-defmethod-with-context-ok ()
+  "Checkdoc should ignore context specializers in a cl-defmethod."
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    ;; A context specializer is used to select the correct method but
+    ;; doesn't have to appear in the docstring:
+    (insert "(cl-defmethod foo (a &context (global-var (eql foo))) \"Return A.\")")
+    (checkdoc-defun)))
+
 (ert-deftest checkdoc-tests--next-docstring ()
   "Checks that the one-argument form of `defvar' works.
 See the comments in Bug#24998."
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* bug#37063: 26.2.90; Problems with recent CL support in checkdoc
  2019-08-17 11:41 bug#37063: 26.2.90; Problems with recent CL support in checkdoc Damien Cassou
@ 2019-09-30 16:52 ` dick.r.chiang
  2019-10-10  0:21   ` Lars Ingebrigtsen
  2019-10-10  0:22 ` Lars Ingebrigtsen
  1 sibling, 1 reply; 7+ messages in thread
From: dick.r.chiang @ 2019-09-30 16:52 UTC (permalink / raw)
  To: 37063

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: checkdoc fix --]
[-- Type: text/x-diff, Size: 2160 bytes --]

From c97b08a1d6df46f674a18ab83ae86dc6a5ad3aa0 Mon Sep 17 00:00:00 2001
From: dickmao <none>
Date: Mon, 30 Sep 2019 10:04:49 -0400
Subject: [PATCH] ; checkdoc identifying formal args

* lisp/emacs-lisp/checkdoc.el (checkdoc-defun-info):
* test/lisp/emacs-lisp/checkdoc-tests.el: (Bug#37063)
---
 lisp/emacs-lisp/checkdoc.el            |  5 ++---
 test/lisp/emacs-lisp/checkdoc-tests.el | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/lisp/emacs-lisp/checkdoc.el b/lisp/emacs-lisp/checkdoc.el
index 51fb75da69..6c40bdf632 100644
--- a/lisp/emacs-lisp/checkdoc.el
+++ b/lisp/emacs-lisp/checkdoc.el
@@ -1952,11 +1952,10 @@ checkdoc-defun-info
 	;; new obarray.
 	(if (not (listp lst)) (setq lst nil))
 	(unless is-advice
-          ;; lst here can be something like ((foo bar) baz) from
+          ;; (car lst) can be something like ((foo bar) baz) from
           ;; cl-lib methods; flatten it:
-          (setq lst (flatten-tree lst))
 	  (while lst
-	    (setq ret (cons (symbol-name (car lst)) ret)
+	    (setq ret (cons (symbol-name (car (flatten-tree (car lst)))) ret)
 		  lst (cdr lst)))))
       (nreverse ret))))
 
diff --git a/test/lisp/emacs-lisp/checkdoc-tests.el b/test/lisp/emacs-lisp/checkdoc-tests.el
index 1cefc4c366..b3cc943ac0 100644
--- a/test/lisp/emacs-lisp/checkdoc-tests.el
+++ b/test/lisp/emacs-lisp/checkdoc-tests.el
@@ -50,4 +50,20 @@ checkdoc-tests--next-docstring
     (should (looking-at-p "\"baz\")"))
     (should-not (checkdoc-next-docstring))))
 
+(ert-deftest checkdoc-tests--cl-defun ()
+  "Identify formal arguments from arbitary lisp code."
+  (with-temp-buffer
+    (let ((checkdoc-autofix-flag 'never))
+      (emacs-lisp-mode)
+      (insert "(cl-defun foo(&key bar &aux (baz (baz bar))) \"BAR BAZ.\")")
+      (should-not (checkdoc-defun)))))
+
+(ert-deftest checkdoc-tests--cl-defmethod ()
+  "Identify formal arguments from object types."
+  (with-temp-buffer
+    (let ((checkdoc-autofix-flag 'never))
+      (emacs-lisp-mode)
+      (insert "(cl-defmethod foo((a list)) \"Return A.\")")
+      (should-not (checkdoc-defun)))))
+
 ;;; checkdoc-tests.el ends here
-- 
2.23.0






^ permalink raw reply related	[flat|nested] 7+ messages in thread

* bug#37063: 26.2.90; Problems with recent CL support in checkdoc
  2019-09-30 16:52 ` dick.r.chiang
@ 2019-10-10  0:21   ` Lars Ingebrigtsen
  0 siblings, 0 replies; 7+ messages in thread
From: Lars Ingebrigtsen @ 2019-10-10  0:21 UTC (permalink / raw)
  To: dick.r.chiang; +Cc: 37063

dick.r.chiang@gmail.com writes:

> diff --git a/lisp/emacs-lisp/checkdoc.el b/lisp/emacs-lisp/checkdoc.el
> index 51fb75da69..6c40bdf632 100644
> --- a/lisp/emacs-lisp/checkdoc.el
> +++ b/lisp/emacs-lisp/checkdoc.el
> @@ -1952,11 +1952,10 @@ checkdoc-defun-info
>  	;; new obarray.
>  	(if (not (listp lst)) (setq lst nil))
>  	(unless is-advice
> -          ;; lst here can be something like ((foo bar) baz) from
> +          ;; (car lst) can be something like ((foo bar) baz) from
>            ;; cl-lib methods; flatten it:
> -          (setq lst (flatten-tree lst))
>  	  (while lst
> -	    (setq ret (cons (symbol-name (car lst)) ret)
> +	    (setq ret (cons (symbol-name (car (flatten-tree (car lst)))) ret)
>  		  lst (cdr lst)))))
>        (nreverse ret))))

Thank you; I've now applied your patch here since it seems to fix the
use cases reported.  However, I didn't include the tests in your patch,
since they seemed to cover the same cases as Damien's (and they were
more extensive, so I applied those instead).

Besides, it doesn't look like you have copyright assignment papers on
file, and for larger patches we require copyright assignments to the
FSF.  If you plan on submitting further patches in the future, it might
be a good idea to do the paperwork now -- would you be willing to do so?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#37063: 26.2.90; Problems with recent CL support in checkdoc
  2019-08-17 11:41 bug#37063: 26.2.90; Problems with recent CL support in checkdoc Damien Cassou
  2019-09-30 16:52 ` dick.r.chiang
@ 2019-10-10  0:22 ` Lars Ingebrigtsen
  2019-10-10  4:15   ` Damien Cassou
  1 sibling, 1 reply; 7+ messages in thread
From: Lars Ingebrigtsen @ 2019-10-10  0:22 UTC (permalink / raw)
  To: Damien Cassou; +Cc: Alex Branham, 37063

Damien Cassou <damien@cassou.me> writes:

> In the context of bug#37034, some initial support for CL functions and
> methods was added to checkdoc.  On my code, the changes make checkdoc
> trigger warnings on perfectly valid code IMO. For example,
>
>   (cl-defmethod foo ((a list)) "Return A.")
>
> Checkdoc complains that LIST should be described. I disagree with that,
> list is a type, not a argument name.
>
> I attach to this bug report a patch introducing test cases to
> checkdoc-tests.el. If it's not possible to implement full support for CL
> I prefer that checkdoc doesn't complain even if there is a bug in the
> docstring rather than checkdoc complains when the docstring is correct.

After a patch was submitted that fixed most of the cases (except the
&context and &aux ones, I believe), I've applied your checkdoc-tests.el
patch (except those two functions).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#37063: 26.2.90; Problems with recent CL support in checkdoc
  2019-10-10  0:22 ` Lars Ingebrigtsen
@ 2019-10-10  4:15   ` Damien Cassou
  2019-10-10 16:45     ` Glenn Morris
  0 siblings, 1 reply; 7+ messages in thread
From: Damien Cassou @ 2019-10-10  4:15 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Alex Branham, 37063

Lars Ingebrigtsen <larsi@gnus.org> writes:
> After a patch was submitted that fixed most of the cases (except the
> &context and &aux ones, I believe), I've applied your checkdoc-tests.el
> patch (except those two functions).

thank you!

-- 
Damien Cassou

"Success is the ability to go from one failure to another without
losing enthusiasm." --Winston Churchill





^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#37063: 26.2.90; Problems with recent CL support in checkdoc
  2019-10-10  4:15   ` Damien Cassou
@ 2019-10-10 16:45     ` Glenn Morris
  2019-10-11  5:48       ` Lars Ingebrigtsen
  0 siblings, 1 reply; 7+ messages in thread
From: Glenn Morris @ 2019-10-10 16:45 UTC (permalink / raw)
  To: Damien Cassou; +Cc: Lars Ingebrigtsen, 37063, Alex Branham


Test failures:

https://hydra.nixos.org/build/102805902





^ permalink raw reply	[flat|nested] 7+ messages in thread

* bug#37063: 26.2.90; Problems with recent CL support in checkdoc
  2019-10-10 16:45     ` Glenn Morris
@ 2019-10-11  5:48       ` Lars Ingebrigtsen
  0 siblings, 0 replies; 7+ messages in thread
From: Lars Ingebrigtsen @ 2019-10-11  5:48 UTC (permalink / raw)
  To: Glenn Morris; +Cc: Damien Cassou, Alex Branham, 37063

Glenn Morris <rgm@gnu.org> writes:

> Test failures:
>
> https://hydra.nixos.org/build/102805902

Should be fixed now.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-10-11  5:48 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-17 11:41 bug#37063: 26.2.90; Problems with recent CL support in checkdoc Damien Cassou
2019-09-30 16:52 ` dick.r.chiang
2019-10-10  0:21   ` Lars Ingebrigtsen
2019-10-10  0:22 ` Lars Ingebrigtsen
2019-10-10  4:15   ` Damien Cassou
2019-10-10 16:45     ` Glenn Morris
2019-10-11  5:48       ` Lars Ingebrigtsen

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.