Re: [PATCH 2/2] emacs: Allow tagging regions in notmuch-tree

[David Bremner <david@tethera.net>]

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 <mail@ambrevar.xyz> 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.