[PATCH] python: use absolute import for SOVERSION

[notmuch-archives.git] / 04 / 325c05be95ab931f672e0885726fdf6a63f4e0

Return-Path: <daniel@schoepe.org>\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 87190429E54\r
        for <notmuch@notmuchmail.org>; Sat, 21 Jan 2012 16:39:39 -0800 (PST)\r
X-Virus-Scanned: Debian amavisd-new at olra.theworths.org\r
X-Spam-Flag: NO\r
X-Spam-Score: -0.8\r
X-Spam-Level: \r
X-Spam-Status: No, score=-0.8 tagged_above=-999 required=5\r
        tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1,\r
        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 lnn9Ki4uFYCD for <notmuch@notmuchmail.org>;\r
        Sat, 21 Jan 2012 16:39:38 -0800 (PST)\r
Received: from mail-ey0-f181.google.com (mail-ey0-f181.google.com\r
        [209.85.215.181]) (using TLSv1 with cipher RC4-SHA (128/128 bits))\r
        (No client certificate requested)\r
        by olra.theworths.org (Postfix) with ESMTPS id 986DE429E40\r
        for <notmuch@notmuchmail.org>; Sat, 21 Jan 2012 16:39:37 -0800 (PST)\r
Received: by eaal1 with SMTP id l1so933034eaa.26\r
        for <notmuch@notmuchmail.org>; Sat, 21 Jan 2012 16:39:35 -0800 (PST)\r
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=schoepe.org; s=google;\r
        h=from:to:subject:in-reply-to:references:user-agent:date:message-id\r
        :mime-version:content-type:x-gm-message-state;\r
        bh=WFqcOlzp7R+t7ZE+qSVRyIDyHIgOW/9AcLFHLSv+7o4=;\r
        b=kRgu2e648by1ahJu4OOcrrJjZRxLTIKVxiSgQuzqyTxwPyho2jSNH9+wKnkL2cQ1x9\r
        kJhLewDAZVvuK4QDIL0BeBb+g0rd4ESxwevUjWi6AU4RgmBCnofdi31JDStTzPZmFTDR\r
        g2vqouOO55yuaKhDSy36uc7RG3SfToYFQcnzU=\r
Received: by 10.213.34.17 with SMTP id j17mr600473ebd.84.1327192774811;\r
        Sat, 21 Jan 2012 16:39:34 -0800 (PST)\r
Received: from localhost (dslb-188-107-198-094.pools.arcor-ip.net.\r
        [188.107.198.94])\r
        by mx.google.com with ESMTPS id t59sm32567809eeh.10.2012.01.21.16.39.31\r
        (version=TLSv1/SSLv3 cipher=OTHER);\r
        Sat, 21 Jan 2012 16:39:32 -0800 (PST)\r
From: Daniel Schoepe <daniel@schoepe.org>\r
To: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>, notmuch@notmuchmail.org\r
Subject: Re: [PATCH v6 1/2] emacs: User-defined sections in notmuch-hello\r
In-Reply-To: <87aa6vvodi.fsf@gmail.com>\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
        <87aa6vvodi.fsf@gmail.com>\r
User-Agent: Notmuch/0.11+62~g888dddb (http://notmuchmail.org) Emacs/24.0.92.1\r
        (x86_64-pc-linux-gnu)\r
Date: Sun, 22 Jan 2012 01:39:26 +0100\r
Message-ID: <87d3ac8ta9.fsf@schoepe.localhost>\r
MIME-Version: 1.0\r
Content-Type: multipart/signed; boundary="=-=-=";\r
        micalg=pgp-sha1; protocol="application/pgp-signature"\r
X-Gm-Message-State:\r
 ALoCoQl9tmpXbO1MF/J5u1hIZsQY42u4KX9g1LZNhhD8l+Q/5kCrL5zaXLkEBPBUqC9PifAcu0Ol\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: Sun, 22 Jan 2012 00:39:39 -0000\r
\r
--=-=-=\r
Content-Type: text/plain\r
Content-Transfer-Encoding: quoted-printable\r
\r
On Wed, 14 Dec 2011 07:11:21 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmai=\r
l.com> wrote:\r
> Hi Daniel.\r
>=20\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
>=20\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
>=20\r
> As we already discussed on IRC, there are some trailing whitespaces to\r
> cleanup.\r
>=20\r
> Here is the review:\r
>=20\r
> +(defvar notmuch-custom-section-options\r
>=20\r
> s/notmuch-custom-section-options/notmuch-hello-custom-section-options/\r
> for consistency?\r
\r
Agreed.\r
\r
>=20\r
> +    (:filter-count (string :tag "Different filter message counts"))\r
>=20\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
This option is for displaying a different number of messages next to the\r
button, than actually match the query the button links to. I think this is\r
something that was requested a while ago in the context of my patch that\r
added notmuch-hello-tag-list-make-query.\r
\r
> +    (:initially-hidden (const :tag "Hide this on startup?" t))\r
>=20\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
Agreed.\r
\r
> Should the default be to show all sections?\r
\r
That's the default I'd prefer, since I want to see most of the section\r
I define by default. If you have some less bike-shedy arguments for\r
changing this, I'm all ears.\r
\r
>=20\r
> +    (:hide-if-empty (const :tag "Hide if empty" t)))\r
>=20\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
Okay, I'll clarify this in the next version.\r
\r
> +  `(list :tag ""\r
> +      (const :tag "" notmuch-hello-insert-query-list)\r
>=20\r
> Do we need to explicitly specify empty tags?  Aren't they empty by\r
> default?\r
\r
It displays the symbol after the const if this is missing.\r
\r
>=20\r
> +  :tag "Customized tag-list (see docstring for details)"\r
> +  :tag "Customized queries section (see docstring for details)"\r
>=20\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
Since this is mainly displayed in the drop-down menu for the section\r
type in the customize page for notmuch-hello-sections (or\r
customize-group 'notmuch), references a) don't work there and b)\r
usually would point to the same thing as the user is currently editing.\r
\r
> Please s/Customized tag-list/Customized tag-list section/ everywhere for\r
> consistency (or remove section from "Customized queries section").\r
\r
Done.\r
\r
>=20\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
>=20\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
>=20\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
>=20\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
>=20\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
>=20\r
>   [ details about customized tag-list and queries sections ]\r
>=20\r
> This is just a draft.  Feel free to use it or ignore it.\r
\r
Thanks, that is quite a bit clearer than what I wrote.\r
\r
> + For convenience an element can also be\r
>=20\r
> Remove space the leading space and do `fill-paragraph'.\r
>=20\r
> +         (function :tag "Custom function"))))\r
>=20\r
> Perhaps "Custom section" would be more accurate?\r
\r
Yes, it would.\r
\r
>=20\r
> +  "Button at position of point before rebuilding the notmuch-buffer\r
>=20\r
> Missing dot at the end.\r
>=20\r
> s/Button/Button text/?\r
>=20\r
> +This variable contains the string of the button, if any, the\r
>=20\r
> s/the string/text/ or label?\r
>=20\r
> +rebuilt. This is never actually set globally and defined as a\r
>=20\r
> s/is never actually set/should never be set/?\r
\r
Sounds good.\r
\r
>=20\r
> +(defvar notmuch-hello-hidden-sections nil\r
> +  "List of query section titles whose contents are hidden")\r
>=20\r
> Is this really for query sections only?\r
\r
No, good catch.\r
\r
>=20\r
> Does this duplicate :initially-hidden option from\r
> notmuch-custom-section-options?\r
\r
This is actually for keeping track of which sections the user chose to\r
hide via the "[hide]"-button. Changing the corresponding field\r
:initially-hidden field in notmuch-sections is not possible for\r
arbitrary sections, i.e. functions and would alter what the users for\r
customize. Also, exiting notmuch and then starting it again would no\r
longer do what the user set in his configuration, if he didn't want to\r
hide it in the beginning, but then clicked on [hide] later.\r
\r
>=20\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
Given that all the variables are prefixed with notmuch-hello already, I\r
don't think that the polluting the namespace is really a problem. And\r
given that section functions would have to access target, I'd rather\r
keep it in a separate variable so people who write a new type of section\r
don't have to inspect some detail in a variable whose other components\r
are internal implementation details (this is the case for first-run and\r
hidden-sections). If this actually becomes a problem we can still\r
move them there later.\r
\r
>=20\r
> `notmuch-hello-filtered-query':\r
>=20\r
> +      (and result (concat query " and (" result ")"))))\r
>=20\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
The entry is already hidden if the function returns nil, because of the\r
`and'. I agree that it's simpler though, so I changed it.\r
\r
>=20\r
> Query should be put in ().\r
>=20\r
> +    (concat query " and (" filter ")"))\r
>=20\r
> Same here.\r
>=20\r
> +   (t (concat query))))\r
>=20\r
> Why do we need concat here?\r
\r
Fixed.\r
\r
>=20\r
> `notmuch-hello-query-counts':\r
>=20\r
> +             (notmuch-hello-filtered-query (cdr query-and-count)\r
> +                                           (or (plist-get options :filter-count)\r
> +                                              (plist-get options :filter)))))))\r
>=20\r
> and\r
>=20\r
> +        (list name (notmuch-hello-filtered-query\r
> +                    (car query-and-count) (plist-get options :filter))\r
> +              message-count))))\r
>=20\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
Yes, I handled that by moving all the code for that to -query-counts,\r
like you suggested in your other mail.\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
>=20\r
> -       reordered-list)\r
> +       searches)\r
>=20\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
It was definitely unintended and changes the default behavior, which is\r
to have the strings sorted within the columns instead of the rows.\r
\r
>=20\r
> - "Major mode for convenient notmuch navigation. This is your entry porta=\r
l into notmuch.\r
> +  "Major mode for convenient notmuch navigation. This is your entry port=\r
al into notmuch.\r
>=20\r
> Please revert.\r
>=20\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
>=20\r
> Please revert.  The commented out line may be removed in a separate patch.\r
>=20\r
> `notmuch-hello-generate-tag-alist':\r
>=20\r
> +                  (list tag (notmuch-hello-filtered-query tag filter-query)\r
>=20\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
>=20\r
> +                (cons tag (notmuch-hello-filtered-query\r
> +                           (concat "tag:" tag) filter-query))))))\r
>=20\r
> Same as above: use the query variable.\r
>=20\r
> `notmuch-hello-insert-saved-searches':\r
>=20\r
> Looks like we do not need both `final-target-pos'.  Can we just return\r
> `found-target-pos'?\r
>=20\r
> `notmuch-hello-insert-search':\r
>=20\r
> +  (insert "\n"))\r
>=20\r
> Should this be `widget-insert'?\r
>=20\r
> Note that there are changes in master that need to be merged into\r
> `notmuch-hello-insert-search' during rebase.\r
>=20\r
> `notmuch-hello-insert-searches':\r
>=20\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
I moved it to notmuch-hello-insert-tags-section, which actually\r
does handle those option and is a high-level function that will\r
probably be used a lot more by users.\r
\r
>=20\r
> +       (searches (apply 'notmuch-hello-query-counts query-alist options)))\r
>=20\r
> Why do we need `apply' here?\r
\r
Because we want each item in `options' to be passed as an individual\r
argument. Note that apply is a bit peculiar about its last argument.\r
\r
> `notmuch-hello-insert-tags-section':\r
>=20\r
> +  "Insert a section displaying all tags and message counts for each.\r
>=20\r
> Perhaps s/and message counts for each/with message counts/?\r
>=20\r
> `notmuch-hello-insert-inbox':\r
>=20\r
> Perhaps change docstring to something more consistent with other\r
> notmuch-hello-insert-* functions?  E.g.:\r
>=20\r
>   Insert a section displaying saved search and inbox messages for each\r
>   tag.\r
\r
Changed, thanks.\r
\r
>=20\r
> +                               (notmuch-hello-generate-tag-alist))\r
> +                              :filter "tag:inbox"))\r
>=20\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
>=20\r
> `notmuch-hello-insert-alltags':\r
>=20\r
> Missing dot at the end of docstring.\r
>=20\r
> Perhaps s/and associated message counts/with message counts/?\r
>=20\r
> `notmuch-hello':\r
>=20\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
>=20\r
> Please revert.\r
>=20\r
>    (if (not notmuch-saved-searches)\r
> -    (setq notmuch-saved-searches (notmuch-saved-searches)))\r
> +      (setq notmuch-saved-searches (notmuch-saved-searches)))\r
>=20\r
> Please revert.\r
>=20\r
> +    (setq notmuch-hello-first-run nil)))\r
>=20\r
> Please move this statement to the top level.  There is no need for it\r
> to be inside let.\r
\r
Fixed.\r
\r
I'll send my current version shortly, which does not yet include\r
Michal's performance improvement, because there probably still be a few\r
rough edges. The performance improvement could also be put in separate\r
patch so I don't have to keep rebasing this uncomfortably big patch for\r
much longer.\r
\r
Thank you for your very detailed review.\r
\r
Cheers,\r
Daniel\r
\r
--=-=-=\r
Content-Type: application/pgp-signature\r
\r
-----BEGIN PGP SIGNATURE-----\r
Version: GnuPG v1.4.11 (GNU/Linux)\r
\r
iQIcBAEBAgAGBQJPG1q+AAoJEIaTAtce+Z+J6IEP/3Ri3FqBe5x7HAB5gmlPvWdQ\r
Kw11Bt9IFbQCmmtvBy8IOgFddWyeu/zaFr/wyHkoqvRpg77t2k5kj9FP2jqTPHQf\r
nETumv4KezLOxqcZfnfUgOmcibCGyDgTxLHBlmMXGX0/V/cvCBiBvPwgVzsIQyYC\r
X5TJpedvjDzqQc8y8Xit/DJ5JsKQxRIStnRkNRahxp4R/dMcNVai3aGNHw3xnKAv\r
IMro9CiTmshi9O2PkXnDpYxNKD5IrjhmTlfffsPNWlPM4d70miUyUTHxhmpre7xn\r
sj9OKqmvLkhrdgVsO9CdSyF7YUWWYu2Q3MbI+TcEbgL3lkbY8GLa6y4b5Zm/0QIt\r
RQYfkUf7EqR6IMdJ3n/ApEP9/PX6LkutNWh0DtYq3HYjnlBnzEb4V/7bWrnSoj5b\r
no7UkenZrYUrTDUBrNkG4kOIyQYTSe5CgR5m4F4N0vHi9r2fNvnBKE8X8D4x5W2Z\r
VhpGVzEp36CtAIH7txyiGkYKbgP4tiFybNlrnVU+qCEVHmkvHjW44dLza382E7K7\r
GzED/L8YdQTxAZCCkFrvxeS/zbn8JoFAQcuGS1sJ2eQbL1v6L10rkp3w7ciGPSSV\r
yU821CrXcb6jA/M6hR9VozkTrh5MhPnWZOclPzDbTTcLcVH+Dn7H3BN6h5PSlUJR\r
bqSJmtCLNUDUsBLtFd/d\r
=OPgO\r
-----END PGP SIGNATURE-----\r
--=-=-=--\r