From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by arlo.cworth.org (Postfix) with ESMTP id C47336DE114E for ; Wed, 8 May 2019 04:01:07 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at cworth.org X-Spam-Flag: NO X-Spam-Score: -0.035 X-Spam-Level: X-Spam-Status: No, score=-0.035 tagged_above=-999 required=5 tests=[AWL=-0.034, SPF_PASS=-0.001] autolearn=disabled Received: from arlo.cworth.org ([127.0.0.1]) by localhost (arlo.cworth.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id kvc1ndzwIZET for ; Wed, 8 May 2019 04:01:06 -0700 (PDT) Received: from fethera.tethera.net (fethera.tethera.net [198.245.60.197]) by arlo.cworth.org (Postfix) with ESMTPS id C04246DE10D0 for ; Wed, 8 May 2019 04:01:06 -0700 (PDT) Received: from remotemail by fethera.tethera.net with local (Exim 4.89) (envelope-from ) id 1hOKJp-0002w4-0p; Wed, 08 May 2019 07:01:01 -0400 Received: (nullmailer pid 29505 invoked by uid 1000); Wed, 08 May 2019 11:00:59 -0000 From: David Bremner To: Pierre Neidhardt , notmuch@notmuchmail.org Subject: Re: [PATCH 2/2] emacs: Allow tagging regions in notmuch-tree In-Reply-To: <20190409164745.13497-1-mail@ambrevar.xyz> References: <20190409164712.13198-1-mail@ambrevar.xyz> <20190409164745.13497-1-mail@ambrevar.xyz> Date: Wed, 08 May 2019 08:00:59 -0300 Message-ID: <878svhfbx0.fsf@tethera.net> MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 May 2019 11:01:07 -0000 Thanks for contributing to Notmuch. Some generic comments: 1) Please consider a more comprehensive commit message [1]. The "why" here is maybe obvious, but consider pointing out whether this makes it more consistent with other parts of the UI (or not). Also, a (bit more extended) of the changes is always helpful, both to help read the diff and to warn of any potential breaking changes. 2) Please update doc/notmuch-emacs.rst. You can re-use docstrings; let me know if you need help doing that. We haven't been strict about this is in the past, but the emacs docs are really lagging. 3) We generally want at least one test for a new feature. test/T460-emacs-tree.sh has the existing tree related tests. Again, if you need help with the test suite, let us know. 4) Please use notmuch-tree-- as a prefix for new private symbols that users should not rely on. I'm not sure about which symbols that applies to here. [1] https://notmuchmail.org/contributing/#index5h2 Pierre Neidhardt writes: > +(defun notmuch-tree-foreach-result (beg end fn) > + (lexical-let ((pos (notmuch-tree-result-beginning beg)) As an aside, I'm curious if we can profitably start to use file level lexical-binding for parts of notmuch-emacs. > + (notmuch-tree-foreach-result beg end > + (lambda (pos) > + (save-mark-and-excursion I did wonder if notmuch-tree-foreach-result should be a macro. I'm not sure if the small gain in code compactness from just passing a "body" in the style of notmuch-show--with-currently-shown-message is worth it.