[PATCH 2/2] emacs: Allow tagging regions in notmuch-tree
david at tethera.net
Wed May 8 04:00:59 PDT 2019
Thanks for contributing to Notmuch. Some generic comments:
1) Please consider a more comprehensive commit message . 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
Pierre Neidhardt <mail at 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.
More information about the notmuch