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

[Tomi Ollila <tomi.ollila@iki.fi>]

On Fri, Jul 31 2015, Jan Malakhovski <oxij@oxij.org> wrote:

> The problem with the previous implementation is that different
> versions of python exhaust __iter__() differently and the
> implementation that can be exhausted is not only absolutely unusable
> in the user code, but it also can not be consistently used with both
> python 2.* and 3.*.
>
> This doesn't change the interface.

Few comments:

There seems to be some code move -- to make the review effort easier
those could be in separate patch.

New tests for len() & unicode() usage in test/T390-python.sh would be nice
(first test that succeeds in one and fails in another (as iter exhausted),
marked known broken, then patch which "fixes" it)

(I also thought of "lazily" filling list of filenames when it is first
requested but doing that probably does not make sense). 


Tomi


> ---
>  bindings/python/notmuch/filenames.py | 84 +++++++++++-------------------------
>  1 file changed, 26 insertions(+), 58 deletions(-)
>
> diff --git a/bindings/python/notmuch/filenames.py b/bindings/python/notmuch/filenames.py
> index 229f414..e2b8886 100644
> --- a/bindings/python/notmuch/filenames.py
> +++ b/bindings/python/notmuch/filenames.py
> @@ -65,6 +65,18 @@ class Filenames(Python3StringMixIn):
>      _get.argtypes = [NotmuchFilenamesP]
>      _get.restype = c_char_p
>  
> +    _valid = nmlib.notmuch_filenames_valid
> +    _valid.argtypes = [NotmuchFilenamesP]
> +    _valid.restype = bool
> +
> +    _move_to_next = nmlib.notmuch_filenames_move_to_next
> +    _move_to_next.argtypes = [NotmuchFilenamesP]
> +    _move_to_next.restype = None
> +
> +    _destroy = nmlib.notmuch_filenames_destroy
> +    _destroy.argtypes = [NotmuchFilenamesP]
> +    _destroy.restype = None
> +
>      def __init__(self, files_p, parent):
>          """
>          :param files_p: A pointer to an underlying *notmuch_tags_t*
> @@ -83,68 +95,24 @@ class Filenames(Python3StringMixIn):
>          if not files_p:
>              raise NullPointerError()
>  
> -        self._files_p = files_p
> +        self._list = []
> +        while self._valid(files_p):
> +            file_ = self._get(files_p)
> +            self._move_to_next(files_p)
> +            self._list.append(file_.decode('utf-8', 'ignore'))
> +        self._destroy(files_p)
> +
>          #save reference to parent object so we keep it alive
>          self._parent = parent
>  
>      def __iter__(self):
> -        """ Make Filenames an iterator """
> -        return self
> -
> -    _valid = nmlib.notmuch_filenames_valid
> -    _valid.argtypes = [NotmuchFilenamesP]
> -    _valid.restype = bool
> -
> -    _move_to_next = nmlib.notmuch_filenames_move_to_next
> -    _move_to_next.argtypes = [NotmuchFilenamesP]
> -    _move_to_next.restype = None
> -
> -    def __next__(self):
> -        if not self._files_p:
> -            raise NotInitializedError()
> -
> -        if not self._valid(self._files_p):
> -            self._files_p = None
> -            raise StopIteration
> -
> -        file_ = Filenames._get(self._files_p)
> -        self._move_to_next(self._files_p)
> -        return file_.decode('utf-8', 'ignore')
> -    next = __next__ # python2.x iterator protocol compatibility
> -
> -    def __unicode__(self):
> -        """Represent Filenames() as newline-separated list of full paths
> -
> -        .. note::
> -
> -            This method exhausts the iterator object, so you will not be able to
> -            iterate over them again.
> -        """
> -        return "\n".join(self)
> -
> -    _destroy = nmlib.notmuch_filenames_destroy
> -    _destroy.argtypes = [NotmuchMessageP]
> -    _destroy.restype = None
> -
> -    def __del__(self):
> -        """Close and free the notmuch filenames"""
> -        if self._files_p:
> -            self._destroy(self._files_p)
> +        """Make Filenames an iterator"""
> +        return self._list.__iter__()
>  
>      def __len__(self):
> -        """len(:class:`Filenames`) returns the number of contained files
> +        """len(:class:`Filenames`) returns the number of contained files"""
> +        return self._list.__len__()
>  
> -        .. note::
> -
> -            This method exhausts the iterator object, so you will not be able to
> -            iterate over them again.
> -        """
> -        if not self._files_p:
> -            raise NotInitializedError()
> -
> -        i = 0
> -        while self._valid(self._files_p):
> -            self._move_to_next(self._files_p)
> -            i += 1
> -        self._files_p = None
> -        return i
> +    def __unicode__(self):
> +        """Represent Filenames() as newline-separated list of full paths"""
> +        return "\n".join(self)
> -- 
> 2.4.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch