Hi Thanks for the review On Wed, 30 Oct 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote: > On Wed, Oct 30 2013, Mark Walters <markwalters1009@gmail.com> wrote: > >> This is a more complete version of the move of pick/tree to >> mainline. The previous version was at >> id:1382257699-29860-1-git-send-email-markwalters1009@gmail.com > > This series looks pretty good to me. IMO patches 1-6 are ready... > >> The previous version did not get the autoloading right: it all worked >> on my setup as I had stale files around in my tree which were loaded. >> This versions builds and tests pass at each stage on a fresh clone. > > You and Me seem to have different understanding what "autoload"ing means. Sorry about that; I just meant that it was automatically loaded when notmuch was loaded (that is the user no longer needs an explicit (require 'notmuch-tree) in their .emacs file. I would probably just stick with the require approach (to be consistent with show etc) unless you (or others) prefer the real autoload as you describe. I will fix the commit messages in the next version. Many thanks Mark > To me autoloading means using (autoload ...) function. In patch 7 > ( id:1383087338-10220-8-git-send-email-markwalters1009@gmail.com ) > (require 'notmuch-tree) is used. > > To me it is fine if this approach is taken, but then calling it autoloading > in commit message (in patches 7 and 8) is confusing. > > However, if (autoload ...) syntax were actually used, it could be done > the somewhat the following way: > > * Don't add (require 'notmuch-tree) in notmuch.el (Patch 7) > > * Add (autoload 'notmuch-tree-from-search-current-query "notmuch-tree" "docstring" t) > to notmuch.el (maybe we can live without declare-function ?) > > * Add (autoload 'notmuch-tree "notmuch-tree" "docstring" t) to > notmuch-lib.el. > > * Now Patch 9 is good as it is now. > > * Don't move the functions mentioned in Patch 9 from notmuch-tree.el to > notmuch.el but write autoload (for notmuch-tree-from-search-thread) to > notmuch.el > > * Now Patch 10 is good as it is now. > > * Leave notmuch-tree-from-show-current-query in notmuch-tree.el, and add > autoload for it in notmuch-show.el. Now you can also drop > (declare-function notmuch-tree ...) out from notmuch-show.el (Patch 11). > > > Tomi > > >> The diffstat at the bottom of this message is huge, so I include the >> rather more informative output from git diff -stat -M here. >> >> Apart from the first patch (which moves 80 lines from notmuch.el to notmuch-lib.el) the total is: >> emacs/Makefile.local | 1 + >> emacs/notmuch-lib.el | 1 + >> emacs/notmuch-show.el | 10 ++++ >> {contrib/notmuch-pick => emacs}/notmuch-tree.el | 47 ++----------------- >> emacs/notmuch.el | 16 +++++++ >> {contrib/notmuch-pick/test => test}/emacs-tree | 13 ----- >> test/notmuch-test | 1 + >> .../tree.expected-output/notmuch-tree-show-window | 0 >> .../notmuch-tree-single-thread | 0 >> .../tree.expected-output/notmuch-tree-tag-inbox | 0 >> .../notmuch-tree-tag-inbox-tagged | 0 >> .../notmuch-tree-tag-inbox-thread-tagged | 0 >> 12 files changed, 34 insertions(+), 55 deletions(-) >> >> >> Obviously this clashes with the remap/help series at >> id:1382821479-23384-1-git-send-email-markwalters1009@gmail.com but >> rebasing either way would be easy. >> >> Best wishes >> >> Mark >> >> >> >> >> >> Mark Walters (11): >> emacs: move notmuch-help to lib >> emacs: tree: remove unneeded declarations >> emacs: move notmuch-tree from contrib to mainline >> emacs: add tree to the makefile >> test: move emacs-tree test into mainline >> emacs: tree: remove test for emacs from tree test >> emacs: minimal change to autoload notmuch-tree >> test: tree: remove require from tests >> emacs: move search based tree functions to notmuch.el >> emacs: add z to common keymap >> emacs: move the show entry to tree into show.el >> >> contrib/notmuch-pick/notmuch-tree.el | 951 -------------------- >> contrib/notmuch-pick/test/emacs-tree | 210 ----- >> .../tree.expected-output/notmuch-tree-show-window | 40 - >> .../notmuch-tree-single-thread | 6 - >> .../tree.expected-output/notmuch-tree-tag-inbox | 53 -- >> .../notmuch-tree-tag-inbox-tagged | 53 -- >> .../notmuch-tree-tag-inbox-thread-tagged | 53 -- >> emacs/Makefile.local | 1 + >> emacs/notmuch-lib.el | 88 ++ >> emacs/notmuch-show.el | 10 + >> emacs/notmuch-tree.el | 914 +++++++++++++++++++ >> emacs/notmuch.el | 103 +-- >> test/emacs-tree | 197 ++++ >> test/notmuch-test | 1 + >> test/tree.expected-output/notmuch-tree-show-window | 40 + >> .../notmuch-tree-single-thread | 6 + >> test/tree.expected-output/notmuch-tree-tag-inbox | 53 ++ >> .../notmuch-tree-tag-inbox-tagged | 53 ++ >> .../notmuch-tree-tag-inbox-thread-tagged | 53 ++ >> 19 files changed, 1432 insertions(+), 1453 deletions(-) >> delete mode 100644 contrib/notmuch-pick/notmuch-tree.el >> delete mode 100755 contrib/notmuch-pick/test/emacs-tree >> delete mode 100644 contrib/notmuch-pick/test/tree.expected-output/notmuch-tree-show-window >> delete mode 100644 contrib/notmuch-pick/test/tree.expected-output/notmuch-tree-single-thread >> delete mode 100644 contrib/notmuch-pick/test/tree.expected-output/notmuch-tree-tag-inbox >> delete mode 100644 contrib/notmuch-pick/test/tree.expected-output/notmuch-tree-tag-inbox-tagged >> delete mode 100644 contrib/notmuch-pick/test/tree.expected-output/notmuch-tree-tag-inbox-thread-tagged >> create mode 100644 emacs/notmuch-tree.el >> create mode 100755 test/emacs-tree >> create mode 100644 test/tree.expected-output/notmuch-tree-show-window >> create mode 100644 test/tree.expected-output/notmuch-tree-single-thread >> create mode 100644 test/tree.expected-output/notmuch-tree-tag-inbox >> create mode 100644 test/tree.expected-output/notmuch-tree-tag-inbox-tagged >> create mode 100644 test/tree.expected-output/notmuch-tree-tag-inbox-thread-tagged >> >> -- >> 1.7.9.1 >> >> _______________________________________________ >> notmuch mailing list >> notmuch@notmuchmail.org >> http://notmuchmail.org/mailman/listinfo/notmuch