From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#66032: [PATCH] Inline advice documentation into advised function's docstring, after all Date: Sat, 16 Sep 2023 11:13:30 -0400 Message-ID: References: Reply-To: Stefan Monnier Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="30878"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: 66032@debbugs.gnu.org, drew.adams@oracle.com To: Jens Schmidt Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sat Sep 16 17:14:17 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1qhWzw-0007r0-Mr for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 16 Sep 2023 17:14:17 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qhWzd-0006bq-1E; Sat, 16 Sep 2023 11:13:57 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qhWzb-0006bL-8X for bug-gnu-emacs@gnu.org; Sat, 16 Sep 2023 11:13:55 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qhWzb-0004Lo-0V for bug-gnu-emacs@gnu.org; Sat, 16 Sep 2023 11:13:55 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qhWzh-0000pg-KW for bug-gnu-emacs@gnu.org; Sat, 16 Sep 2023 11:14:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Monnier Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 16 Sep 2023 15:14:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 66032 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 66032-submit@debbugs.gnu.org id=B66032.16948772373182 (code B ref 66032); Sat, 16 Sep 2023 15:14:01 +0000 Original-Received: (at 66032) by debbugs.gnu.org; 16 Sep 2023 15:13:57 +0000 Original-Received: from localhost ([127.0.0.1]:48273 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qhWzc-0000pF-KN for submit@debbugs.gnu.org; Sat, 16 Sep 2023 11:13:57 -0400 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:51870) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qhWzW-0000ow-IK for 66032@debbugs.gnu.org; Sat, 16 Sep 2023 11:13:54 -0400 Original-Received: from pmg3.iro.umontreal.ca (localhost [127.0.0.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id 95E724427F8; Sat, 16 Sep 2023 11:13:37 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1694877211; bh=usD6z2Xq0d5NpirXAnfWD6qI8g/RYRgenyhA26Pef/0=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=RXPPhk37PunpG8MxdMonuim0DDz0BEVXirhuBc6vij7nNas4cFSIuWUT4lED7ORRV NerosKg165p8X2z/O8wjsjN5TiSpclJzdHxzOz580NtAR+kZPAiU52nNZm6uxKZCEE 6zXCaA9gsBZEr6Ya+YRowuwxEChtObEobZYJPF7DRk2syWFXYP1adghBxrRO5hLKiD Na4EZ/E/4+GYNiYO3ZI3eubfCXDM5wEN2q8K33roK+7a5vkYxK6Fx1QogRUb7KYJvU oTBk9Pay4pGUzJQuI+85bQgniJN9IMnA02P/uxcSmAzwfGnmQ/9kM75kCYMG5bj/3u IlInppDrPnirQ== Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg3.iro.umontreal.ca (Proxmox) with ESMTP id 6F6FA4427F9; Sat, 16 Sep 2023 11:13:31 -0400 (EDT) Original-Received: from pastel (unknown [104.247.237.102]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 46E6312023D; Sat, 16 Sep 2023 11:13:31 -0400 (EDT) In-Reply-To: (Jens Schmidt's message of "Sat, 16 Sep 2023 14:57:33 +0200") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:270627 Archived-At: > I'm aware that I'm late to the party (got unstuck from Emacs 23 > only recently) and that there have been some reports on that > already (bug#14734 and merged). :-) > But at least on one occasion Stefan has asked for a patch, and I > haven't seen yet patches that got declined. It does happen, tho. > ------------------------- snip ------------------------- > sm-test1 is a Lisp macro. > > (sm-test1 A) > > This function is advised. > > This macro has :override advice: No documentation > > This is an :override advice, which means that =E2=80=98sm-test1=E2=80=99 = isn=E2=80=99t > run at all, and the documentation below may be irrelevant. > > This macro has :around advice: No documentation > > [back] > ------------------------- snip ------------------------- Hmm... why does it say "This function is advised" here where the rest talks about a macro instead? [ This is not your fault, but the handling of :override isn't right currently, so if you could fix it while you're there, it would be great: the problem is that the mention of the specific override advice is placed at the beginning and all the others at the end, which makes it impossible to see the relative order of the :override advice w.r.t to the others. ] > What do you think? I got used to the single line where I have to click to get more info, so I'm not the target audience, but see comments below. > + (if (symbolp fun) > + (if doc > + (concat > + (format-message "`%S'\n" fun) > + ;; Remove first line possibly added by > + ;; `ad-make-single-advice-docstring' for > + ;; legacy advices. > + (if (string-match > + "^\\(?:Before\\|Around\\|After\\)-advice `.*?':\n" > + doc) > + (substring doc (match-end 0)) > + doc)) > + (format-message "`%S'." fun)) We don't want code to cater to the old `advice.el` here. If you don't like that extra line, remove it in `advice.el` instead. But now that `defadvice` is deprecated, I'm not sure it's worth the trouble anyway. The main problem I see, tho, is how to clearly "delimit" the docstring. Maybe we should indent the advice's docstring by two spaces or so? > + (setq name (cdr (assq 'name (advice--props flist)))) > + (if name > + (if doc > + (format "%s\n%s" name doc) > + (format "%s" name)) [ I realize this is not your fault either, but we should say This function has :before advice named "NAME" rather than This function has :before advice: NAME ] Fun situation where your code misfires: (defun sm-foo1 (&rest _) "Hello, this is foo1\n\nhere." nil) (advice-add 'diff-mode :around #'sm-foo1) (advice-add 'sm-foo1 :after #'ignore) makes `C-h f diff-mode RET` show: [...] This mode runs the hook =E2=80=98diff-mode-hook=E2=80=99, as the final = or penultimate step during initialization. =20=20=20=20 This function is advised. =20=20=20=20 This function has :around advice: =E2=80=98sm-foo1=E2=80=99 Hello, this is foo1 =20=20=20=20 here. =20=20=20=20 This function is advised. =20=20=20=20 This function has :after advice: =E2=80=98ignore=E2=80=99 Ignore ARGUMENTS, do nothing, and return nil. This function accepts any number of arguments in ARGUMENTS. Also see =E2=80=98always=E2=80=99. =20=20=20=20 Probably introduced at or before Emacs version 21.1. We can up the ante even further and advise `ignore`: (advice-add 'ignore :after #'sm-foo1) such that `C-h f diff-mode RET` now tells us: Lisp nesting exceeds =E2=80=98max-lisp-eval-depth=E2=80=99 :-) > @@ -155,10 +168,22 @@ advice--make-docstring > (help-add-fundoc-usage > (with-temp-buffer > (when before > + (insert > + (propertize > + (concat "This " (if macrop "macro" "function") " is advised= .") > + 'face 'font-lock-warning-face)) > + (ensure-empty-lines 1) I like this warning, but I don't like its code duplication. The positive side is that I think this line should always come before the main doc (and should be merged with the warning about "... override ... documentation below may be irrelevant" when applicable). Stefan