Re: Python bindings for adoption

[notmuch-archives.git] / d3 / f968464a6b4771bd6cab6b4736d6056c0e9c92

Return-Path: <dmitry.kurochkin@gmail.com>\r
X-Original-To: notmuch@notmuchmail.org\r
Delivered-To: notmuch@notmuchmail.org\r
Received: from localhost (localhost [127.0.0.1])\r
        by olra.theworths.org (Postfix) with ESMTP id 290C7429E26\r
        for <notmuch@notmuchmail.org>; Tue, 13 Dec 2011 19:12:04 -0800 (PST)\r
X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
X-Spam-Flag: NO\r
X-Spam-Score: -0.799\r
X-Spam-Level: \r
X-Spam-Status: No, score=-0.799 tagged_above=-999 required=5\r
        tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,\r
        FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_LOW=-0.7] autolearn=disabled\r
Received: from olra.theworths.org ([127.0.0.1])\r
        by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)\r
        with ESMTP id fCiVnK0tgsrl for <notmuch@notmuchmail.org>;\r
        Tue, 13 Dec 2011 19:12:03 -0800 (PST)\r
Received: from mail-ww0-f45.google.com (mail-ww0-f45.google.com\r
 [74.125.82.45])        (using TLSv1 with cipher RC4-SHA (128/128 bits))        (No client\r
 certificate requested) by olra.theworths.org (Postfix) with ESMTPS id\r
 E4841429E25    for <notmuch@notmuchmail.org>; Tue, 13 Dec 2011 19:12:02 -0800\r
 (PST)\r
Received: by wgbds13 with SMTP id ds13so575066wgb.2\r
        for <notmuch@notmuchmail.org>; Tue, 13 Dec 2011 19:12:00 -0800 (PST)\r
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma;\r
        h=from:to:cc:subject:in-reply-to:references:user-agent:date\r
        :message-id:mime-version:content-type;\r
        bh=OYTCZESx+piR+sNuqs72az1kebqHwOt3wGD4w9YaEVU=;\r
        b=J/ZyUhJLZb23aL3O0ie2Wm/jVpcBVLLjIRMlgDXAIXGrBwsWkmkj43bjBarDYFmhY+\r
        Ugi6F4F53AJh0SoZADLgSmQ3IlD+SPlT62/4a7AHHkfC6rWGqoczkEcbU4vW4ZeqD6gQ\r
        IX8VASxwKStObv/ndPjkKuOFxHfk5+28B49bI=\r
Received: by 10.180.14.69 with SMTP id n5mr1654011wic.13.1323832320258;\r
        Tue, 13 Dec 2011 19:12:00 -0800 (PST)\r
Received: from localhost ([91.144.186.21])\r
        by mx.google.com with ESMTPS id ej17sm1602459wbb.14.2011.12.13.19.11.57\r
        (version=TLSv1/SSLv3 cipher=OTHER);\r
        Tue, 13 Dec 2011 19:11:58 -0800 (PST)\r
From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>\r
To: Daniel Schoepe <daniel@schoepe.org>, notmuch@notmuchmail.org\r
Subject: Re: [PATCH v6 1/2] emacs: User-defined sections in notmuch-hello\r
In-Reply-To: <1318253982-23588-2-git-send-email-daniel@schoepe.org>\r
References: <87ippzysmv.fsf@steelpick.2x.cz>\r
        <1318253982-23588-1-git-send-email-daniel@schoepe.org>\r
        <1318253982-23588-2-git-send-email-daniel@schoepe.org>\r
User-Agent: Notmuch/0.10.2+96~g74e5ae5 (http://notmuchmail.org) Emacs/23.3.1\r
        (x86_64-pc-linux-gnu)\r
Date: Wed, 14 Dec 2011 07:11:21 +0400\r
Message-ID: <87aa6vvodi.fsf@gmail.com>\r
MIME-Version: 1.0\r
Content-Type: text/plain; charset=us-ascii\r
Cc: Daniel Schoepe <daniel.schoepe@googlemail.com>\r
X-BeenThere: notmuch@notmuchmail.org\r
X-Mailman-Version: 2.1.13\r
Precedence: list\r
List-Id: "Use and development of the notmuch mail system."\r
        <notmuch.notmuchmail.org>\r
List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,\r
        <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>\r
List-Archive: <http://notmuchmail.org/pipermail/notmuch>\r
List-Post: <mailto:notmuch@notmuchmail.org>\r
List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>\r
List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,\r
        <mailto:notmuch-request@notmuchmail.org?subject=subscribe>\r
X-List-Received-Date: Wed, 14 Dec 2011 03:12:04 -0000\r
\r
Hi Daniel.\r
\r
I have finished reviewing this patch at last.  Sorry, it is a bit messy.\r
Overall, I like the patch.  It is a very nice improvement.\r
\r
I am sure I have missed some important points, but I guess this is the\r
best I can do right now.  Perhaps I will find more comments for the next\r
version of the patch :)\r
\r
As we already discussed on IRC, there are some trailing whitespaces to\r
cleanup.\r
\r
Here is the review:\r
\r
+(defvar notmuch-custom-section-options\r
\r
s/notmuch-custom-section-options/notmuch-hello-custom-section-options/ for consistency?\r
\r
+    (:filter-count (string :tag "Different filter message counts"))\r
\r
It was not clear to me what this option is for from the docstring.\r
Perhaps something like: "Count query filter, if different from :filter"?\r
\r
+    (:initially-hidden (const :tag "Hide this on startup?" t))\r
\r
"This" refers to section, right?  If yes, let's state it explicitly:\r
"Hide this section on startup".  Also, we should probably remove the\r
question mark, or add it to other options for consistency.\r
\r
Should the default be to show all sections?\r
\r
+    (:hide-if-empty (const :tag "Hide if empty" t)))\r
\r
As I understand, this controls whether the whole sections is visible.\r
It is not clear what "if empty" means.  Does it mean that all queries\r
are empty?  Or all queries are empty and :show-empty-sections is\r
false?  Consider changing to something like: "Hide this section if all\r
queries are empty [and hidden]".\r
\r
+  `(list :tag ""\r
+        (const :tag "" notmuch-hello-insert-query-list)\r
\r
Do we need to explicitly specify empty tags?  Aren't they empty by\r
default?\r
\r
+  :tag "Customized tag-list (see docstring for details)"\r
+  :tag "Customized queries section (see docstring for details)"\r
\r
Perhaps it would be more useful to add reference to\r
`notmuch-hello-sections'?  I.e. "see `notmuch-hello-sections' for\r
details.\r
\r
Please s/Customized tag-list/Customized tag-list section/ everywhere for\r
consistency (or remove section from "Customized queries section").\r
\r
+Each entry of this list should be a function of no arguments that\r
+should return if `notmuch-hello-target' is produced as part of its\r
+output and nil otherwise.\r
\r
Something is missing between "return if".  IMO it is really hard to\r
understand what the function should actually do and what it should\r
return.  Are this functions expected to add section content to current\r
position?  As I understand, the return value indicates whether cursor\r
should be positioned somewhere inside this section.  It is a minor\r
detail, but it is described in the first (and complex sentence) as if\r
it was the most important part.  Consider moving the return and "no\r
arguments" to the 3rd paragraph which describes details about the\r
functions.  I would also swap 2nd and 3rd paragraph.  Smth like:\r
\r
  The list contains functions which are used to construct sections in\r
  notmuch-hello buffer.  When notmuch-hello buffer is constructed,\r
  these functions are run in the order they appear in this list.  Each\r
  function produces a section simply by adding content to the current\r
  buffer.  A section should not end with an empty line, because a\r
  newline will be inserted after each section by `notmuch-hello'.\r
\r
  Each function should take no arguments.  If the produced section\r
  includes `notmuch-hello-target' (i.e. cursor should be positioned\r
  inside this section), the function should return [something].\r
  Otherwise, it should return nil.\r
\r
  For convenience an element can also be a list of the form (FUNC ARG1\r
  ARG2 .. ARGN) in which case FUNC will be applied to the rest of the\r
  list.\r
\r
  [ details about customized tag-list and queries sections ]\r
\r
This is just a draft.  Feel free to use it or ignore it.\r
\r
+ For convenience an element can also be\r
\r
Remove space the leading space and do `fill-paragraph'.\r
\r
+           (function :tag "Custom function"))))\r
\r
Perhaps "Custom section" would be more accurate?\r
\r
+  "Button at position of point before rebuilding the notmuch-buffer\r
\r
Missing dot at the end.\r
\r
s/Button/Button text/?\r
\r
+This variable contains the string of the button, if any, the\r
\r
s/the string/text/ or label?\r
\r
+rebuilt. This is never actually set globally and defined as a\r
\r
s/is never actually set/should never be set/?\r
\r
+(defvar notmuch-hello-hidden-sections nil\r
+  "List of query section titles whose contents are hidden")\r
\r
Is this really for query sections only?\r
\r
Does this duplicate :initially-hidden option from\r
notmuch-custom-section-options?\r
\r
How about adding a global alist variable notmuch-hello-state to store\r
the state needed for section functions?  Currently, it would contain\r
two values: :first-run and :target.  This would allow us to add more\r
state variables in the future without polluting the global namespace.\r
Also, it would make clear what variables are section function are\r
supposed to use and perhaps even change (docstring should explain that\r
of course).\r
\r
`notmuch-hello-filtered-query':\r
\r
+      (and result (concat query " and (" result ")"))))\r
\r
How about using the result as query instead of filter?  I.e. returning\r
the result without adding the query to it.  IMO it is simpler and more\r
flexible.  In particular, that would allow the function to return nil\r
in case the query should not be shown.\r
\r
Query should be put in ().\r
\r
+    (concat query " and (" filter ")"))\r
\r
Same here.\r
\r
+   (t (concat query))))\r
\r
Why do we need concat here?\r
\r
`notmuch-hello-query-counts':\r
\r
+               (notmuch-hello-filtered-query (cdr query-and-count)\r
+                                             (or (plist-get options :filter-count)\r
+                                                (plist-get options :filter)))))))\r
\r
and\r
\r
+          (list name (notmuch-hello-filtered-query\r
+                      (car query-and-count) (plist-get options :filter))\r
+                message-count))))\r
\r
We already handled :filter and :filter-count options in\r
`notmuch-hello-generate-tag-alist'.  We should just use the generated\r
queries passed in query-alist.\r
\r
It seems that `notmuch-hello-query-counts' should handle only\r
:show-empty-searches option.  If that is true, docstring should be\r
updated accordingly.  Also, I think it is better to pass a single\r
:show-empty-searches option as a parameter instead of the whole\r
options plist.\r
\r
-         reordered-list)\r
+         searches)\r
\r
I am not sure if this is a mistake.  I hope it is not, because it\r
allows us to remove some code :) If this change is correct, please\r
make it a separate patch and remove unused reordered-list variable,\r
notmuch-hello-reflect and notmuch-hello-reflect-generate-row\r
functions.  Otherwise, revert the change.\r
\r
- "Major mode for convenient notmuch navigation. This is your entry portal into notmuch.\r
+  "Major mode for convenient notmuch navigation. This is your entry portal into notmuch.\r
\r
Please revert.\r
\r
- (interactive)\r
- (kill-all-local-variables)\r
- (use-local-map notmuch-hello-mode-map)\r
- (setq major-mode 'notmuch-hello-mode\r
-       mode-name "notmuch-hello")\r
- ;;(setq buffer-read-only t)\r
-)\r
-\r
+  (interactive)\r
+  (kill-all-local-variables)\r
+  (use-local-map notmuch-hello-mode-map)\r
+  (setq major-mode 'notmuch-hello-mode\r
+       mode-name "notmuch-hello"))\r
+\r
\r
Please revert.  The commented out line may be removed in a separate patch.\r
\r
`notmuch-hello-generate-tag-alist':\r
\r
+                    (list tag (notmuch-hello-filtered-query tag filter-query)\r
\r
It should be (concat "tag:" tag) instead of tag.  Besides we already\r
have it in the query variable, so just use it.\r
\r
+                  (cons tag (notmuch-hello-filtered-query\r
+                             (concat "tag:" tag) filter-query))))))\r
\r
Same as above: use the query variable.\r
\r
`notmuch-hello-insert-saved-searches':\r
\r
Looks like we do not need both `final-target-pos'.  Can we just return\r
`found-target-pos'?\r
\r
`notmuch-hello-insert-search':\r
\r
+  (insert "\n"))\r
\r
Should this be `widget-insert'?\r
\r
Note that there are changes in master that need to be merged into\r
`notmuch-hello-insert-search' during rebase.\r
\r
`notmuch-hello-insert-searches':\r
\r
if my above comments on `notmuch-hello-query-counts' are correct, the\r
docstring should be fixed because `notmuch-hello-insert-searches' does\r
not handle :filter and :filter-count options.  Would be nice to move\r
this documentation somewhere instead of deleting it.\r
\r
+         (searches (apply 'notmuch-hello-query-counts query-alist options)))\r
\r
Why do we need `apply' here?\r
\r
`notmuch-hello-insert-tags-section':\r
\r
+  "Insert a section displaying all tags and message counts for each.\r
\r
Perhaps s/and message counts for each/with message counts/?\r
\r
`notmuch-hello-insert-inbox':\r
\r
Perhaps change docstring to something more consistent with other\r
notmuch-hello-insert-* functions?  E.g.:\r
\r
  Insert a section displaying saved search and inbox messages for each tag.\r
\r
+                                 (notmuch-hello-generate-tag-alist))\r
+                                :filter "tag:inbox"))\r
\r
If my above comments are correct, then :filter option should be given\r
to `notmuch-hello-generate-tag-alist' instead of\r
`notmuch-hello-insert-searches'.\r
\r
`notmuch-hello-insert-alltags':\r
\r
Missing dot at the end of docstring.\r
\r
Perhaps s/and associated message counts/with message counts/?\r
\r
`notmuch-hello':\r
\r
-  ; Jump through a hoop to get this value from the deprecated variable\r
-  ; name (`notmuch-folders') or from the default value.\r
+                                       ; Jump through a hoop to get this value from the deprecated variable\r
+                                       ; name (`notmuch-folders') or from the default value.\r
\r
Please revert.\r
\r
   (if (not notmuch-saved-searches)\r
-    (setq notmuch-saved-searches (notmuch-saved-searches)))\r
+      (setq notmuch-saved-searches (notmuch-saved-searches)))\r
\r
Please revert.\r
\r
+    (setq notmuch-hello-first-run nil)))\r
\r
Please move this statement to the top level.  There is no need for it\r
to be inside let.\r
\r
\r
Regards,\r
  Dmitry\r