From: Mark Walters <markwalters1009@gmail.com>
To: Tomi Ollila <tomi.ollila@iki.fi>, notmuch@notmuchmail.org
Subject: Re: [PATCH 00/11] Move notmuch-tree to mainline
In-Reply-To: <m2bo26eiy8.fsf@guru.guru-group.fi>
References: <1383087338-10220-1-git-send-email-markwalters1009@gmail.com>
	<m2bo26eiy8.fsf@guru.guru-group.fi>
User-Agent: Notmuch/0.16 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Wed, 30 Oct 2013 18:48:10 +0000
Message-ID: <87d2mm8uj9.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


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