Re: [PATCH 3/3] python/notmuch2: provide binding for collect_tags()

[Floris Bruynooghe <flub@devork.be>]

Hi Micahel,

Thanks for adding this feature!

On Wed 06 Jan 2021 at 10:08 +0100, Michael J. Gruber wrote:
> diff --git a/bindings/python-cffi/notmuch2/_query.py b/bindings/python-cffi/notmuch2/_query.py
> index 1db6ec96..a1310944 100644
> --- a/bindings/python-cffi/notmuch2/_query.py
> +++ b/bindings/python-cffi/notmuch2/_query.py
> @@ -2,6 +2,7 @@ from notmuch2 import _base as base
>  from notmuch2 import _capi as capi
>  from notmuch2 import _errors as errors
>  from notmuch2 import _message as message
> +from notmuch2 import _tags as tags
>  from notmuch2 import _thread as thread
>  
>  
> @@ -66,6 +67,17 @@ class Query(base.NotmuchObject):
>              raise errors.NotmuchError(ret)
>          return count_p[0]
>  
> +    def collect_tags(self):
> +        """Return the tags of messages matching this query."""
> +        msgs_pp = capi.ffi.new('notmuch_messages_t**')
> +        ret = capi.lib.notmuch_query_search_messages(self._query_p, msgs_pp)
> +        if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
> +            raise errors.NotmuchError(ret)
> +        self._msgs_p = msgs_pp[0]
> +        tagset = tags.ImmutableTagSet(
> +            self, '_msgs_p', capi.lib.notmuch_messages_collect_tags)
> +        return tags.ImmutableTagSet._from_iterable(tagset)
> +
>      def threads(self):
>          """Return an iterator over all the threads found by the query."""
>          threads_pp = capi.ffi.new('notmuch_threads_t **')

How this this look on the C level for the memory allocator?  If the
query gets freed the messages also get freed?  In that case indeed the
messages do need to be kept alive together with the query indeed.
Keeping them as an attribute on the query object seems reasonable I
think, they'd be freed at the same time and I don't think this creates a
circular reference.

There's only a small detail though: to get the correct ObjectDestroyed
exceptions you need Query._msgs_p to be a _base.MemoryPoiniter
descriptor, like Query._query_p is currently.  And then you also need to
set it to None in Query._destroy.

It would also be really good to add at least one happy-path test for
this, ensuring that you get the tags in the query.  You could add some
to TestQuery in test_database.py.  The database is created with unread
and inbox tags i think (see conftest.py:75) so the messages should
already have some tags which is sufficient as you're not testing
notmuch's behaviour itself but only that the bindings give you a right
looking tagset (the tagset itself also has tests already, so no need to
test this extensively, just see if you have some tags).

Otherwise LGTM!

Cheers,
Floris