Re: [PATCH] python: fix get_filenames() and make it actually usable

diff --git a/dd/ead8e58f232e2217f70c995c05216fadcaf846 b/dd/ead8e58f232e2217f70c995c05216fadcaf846
new file mode 100644 (file)
index 0000000..d191e4f
--- /dev/null
+++ b/dd/ead8e58f232e2217f70c995c05216fadcaf846
@@ -0,0 +1,193 @@
+Return-Path: <tomi.ollila@iki.fi>\r
+X-Original-To: notmuch@notmuchmail.org\r
+Delivered-To: notmuch@notmuchmail.org\r
+Received: from localhost (localhost [127.0.0.1])\r
+ by arlo.cworth.org (Postfix) with ESMTP id 4ECD16DE0B20\r
+ for <notmuch@notmuchmail.org>; Fri, 31 Jul 2015 02:42:30 -0700 (PDT)\r
+X-Virus-Scanned: Debian amavisd-new at cworth.org\r
+X-Spam-Flag: NO\r
+X-Spam-Score: 0.932\r
+X-Spam-Level: \r
+X-Spam-Status: No, score=0.932 tagged_above=-999 required=5 tests=[AWL=0.280, \r
+ SPF_NEUTRAL=0.652] autolearn=disabled\r
+Received: from arlo.cworth.org ([127.0.0.1])\r
+ by localhost (arlo.cworth.org [127.0.0.1]) (amavisd-new, port 10024)\r
+ with ESMTP id Nn8bcNVIejQ6 for <notmuch@notmuchmail.org>;\r
+ Fri, 31 Jul 2015 02:42:27 -0700 (PDT)\r
+Received: from guru.guru-group.fi (guru.guru-group.fi [46.183.73.34])\r
+ by arlo.cworth.org (Postfix) with ESMTP id 607B76DE0B44\r
+ for <notmuch@notmuchmail.org>; Fri, 31 Jul 2015 02:42:26 -0700 (PDT)\r
+Received: from guru.guru-group.fi (localhost [IPv6:::1])\r
+ by guru.guru-group.fi (Postfix) with ESMTP id CC24B10007F;\r
+ Fri, 31 Jul 2015 12:42:08 +0300 (EEST)\r
+From: Tomi Ollila <tomi.ollila@iki.fi>\r
+To: Jan Malakhovski <oxij@oxij.org>, notmuch@notmuchmail.org\r
+Subject: Re: [PATCH] python: fix get_filenames() and make it actually usable\r
+In-Reply-To: <1438306586-7597-1-git-send-email-oxij@oxij.org>\r
+References: <1438306586-7597-1-git-send-email-oxij@oxij.org>\r
+User-Agent: Notmuch/0.20.2+31~g967a7a3 (http://notmuchmail.org) Emacs/24.3.1\r
+ (x86_64-unknown-linux-gnu)\r
+X-Face: HhBM'cA~<r"^Xv\KRN0P{vn'Y"Kd;zg_y3S[4)KSN~s?O\"QPoL\r
+ $[Xv_BD:i/F$WiEWax}R(MPS`^UaptOGD`*/=@\1lKoVa9tnrg0TW?"r7aRtgk[F\r
+ !)g;OY^,BjTbr)Np:%c_o'jj,Z\r
+Date: Fri, 31 Jul 2015 12:42:08 +0300\r
+Message-ID: <m2lhdwll3j.fsf@guru.guru-group.fi>\r
+MIME-Version: 1.0\r
+Content-Type: text/plain\r
+X-BeenThere: notmuch@notmuchmail.org\r
+X-Mailman-Version: 2.1.18\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: Fri, 31 Jul 2015 09:42:30 -0000\r
+\r
+On Fri, Jul 31 2015, Jan Malakhovski <oxij@oxij.org> wrote:\r
+\r
+> The problem with the previous implementation is that different\r
+> versions of python exhaust __iter__() differently and the\r
+> implementation that can be exhausted is not only absolutely unusable\r
+> in the user code, but it also can not be consistently used with both\r
+> python 2.* and 3.*.\r
+>\r
+> This doesn't change the interface.\r
+\r
+Few comments:\r
+\r
+There seems to be some code move -- to make the review effort easier\r
+those could be in separate patch.\r
+\r
+New tests for len() & unicode() usage in test/T390-python.sh would be nice\r
+(first test that succeeds in one and fails in another (as iter exhausted),\r
+marked known broken, then patch which "fixes" it)\r
+\r
+(I also thought of "lazily" filling list of filenames when it is first\r
+requested but doing that probably does not make sense). \r
+\r
+\r
+Tomi\r
+\r
+\r
+> ---\r
+>  bindings/python/notmuch/filenames.py | 84 +++++++++++-------------------------\r
+>  1 file changed, 26 insertions(+), 58 deletions(-)\r
+>\r
+> diff --git a/bindings/python/notmuch/filenames.py b/bindings/python/notmuch/filenames.py\r
+> index 229f414..e2b8886 100644\r
+> --- a/bindings/python/notmuch/filenames.py\r
+> +++ b/bindings/python/notmuch/filenames.py\r
+> @@ -65,6 +65,18 @@ class Filenames(Python3StringMixIn):\r
+>      _get.argtypes = [NotmuchFilenamesP]\r
+>      _get.restype = c_char_p\r
+>  \r
+> +    _valid = nmlib.notmuch_filenames_valid\r
+> +    _valid.argtypes = [NotmuchFilenamesP]\r
+> +    _valid.restype = bool\r
+> +\r
+> +    _move_to_next = nmlib.notmuch_filenames_move_to_next\r
+> +    _move_to_next.argtypes = [NotmuchFilenamesP]\r
+> +    _move_to_next.restype = None\r
+> +\r
+> +    _destroy = nmlib.notmuch_filenames_destroy\r
+> +    _destroy.argtypes = [NotmuchFilenamesP]\r
+> +    _destroy.restype = None\r
+> +\r
+>      def __init__(self, files_p, parent):\r
+>          """\r
+>          :param files_p: A pointer to an underlying *notmuch_tags_t*\r
+> @@ -83,68 +95,24 @@ class Filenames(Python3StringMixIn):\r
+>          if not files_p:\r
+>              raise NullPointerError()\r
+>  \r
+> -        self._files_p = files_p\r
+> +        self._list = []\r
+> +        while self._valid(files_p):\r
+> +            file_ = self._get(files_p)\r
+> +            self._move_to_next(files_p)\r
+> +            self._list.append(file_.decode('utf-8', 'ignore'))\r
+> +        self._destroy(files_p)\r
+> +\r
+>          #save reference to parent object so we keep it alive\r
+>          self._parent = parent\r
+>  \r
+>      def __iter__(self):\r
+> -        """ Make Filenames an iterator """\r
+> -        return self\r
+> -\r
+> -    _valid = nmlib.notmuch_filenames_valid\r
+> -    _valid.argtypes = [NotmuchFilenamesP]\r
+> -    _valid.restype = bool\r
+> -\r
+> -    _move_to_next = nmlib.notmuch_filenames_move_to_next\r
+> -    _move_to_next.argtypes = [NotmuchFilenamesP]\r
+> -    _move_to_next.restype = None\r
+> -\r
+> -    def __next__(self):\r
+> -        if not self._files_p:\r
+> -            raise NotInitializedError()\r
+> -\r
+> -        if not self._valid(self._files_p):\r
+> -            self._files_p = None\r
+> -            raise StopIteration\r
+> -\r
+> -        file_ = Filenames._get(self._files_p)\r
+> -        self._move_to_next(self._files_p)\r
+> -        return file_.decode('utf-8', 'ignore')\r
+> -    next = __next__ # python2.x iterator protocol compatibility\r
+> -\r
+> -    def __unicode__(self):\r
+> -        """Represent Filenames() as newline-separated list of full paths\r
+> -\r
+> -        .. note::\r
+> -\r
+> -            This method exhausts the iterator object, so you will not be able to\r
+> -            iterate over them again.\r
+> -        """\r
+> -        return "\n".join(self)\r
+> -\r
+> -    _destroy = nmlib.notmuch_filenames_destroy\r
+> -    _destroy.argtypes = [NotmuchMessageP]\r
+> -    _destroy.restype = None\r
+> -\r
+> -    def __del__(self):\r
+> -        """Close and free the notmuch filenames"""\r
+> -        if self._files_p:\r
+> -            self._destroy(self._files_p)\r
+> +        """Make Filenames an iterator"""\r
+> +        return self._list.__iter__()\r
+>  \r
+>      def __len__(self):\r
+> -        """len(:class:`Filenames`) returns the number of contained files\r
+> +        """len(:class:`Filenames`) returns the number of contained files"""\r
+> +        return self._list.__len__()\r
+>  \r
+> -        .. note::\r
+> -\r
+> -            This method exhausts the iterator object, so you will not be able to\r
+> -            iterate over them again.\r
+> -        """\r
+> -        if not self._files_p:\r
+> -            raise NotInitializedError()\r
+> -\r
+> -        i = 0\r
+> -        while self._valid(self._files_p):\r
+> -            self._move_to_next(self._files_p)\r
+> -            i += 1\r
+> -        self._files_p = None\r
+> -        return i\r
+> +    def __unicode__(self):\r
+> +        """Represent Filenames() as newline-separated list of full paths"""\r
+> +        return "\n".join(self)\r
+> -- \r
+> 2.4.1\r
+>\r
+> _______________________________________________\r
+> notmuch mailing list\r
+> notmuch@notmuchmail.org\r
+> http://notmuchmail.org/mailman/listinfo/notmuch\r