[PATCH 1/2] python: add bindings for notmuch_database_get_config{, _list}

classic Classic list List threaded Threaded
6 messages Options
l-m-h l-m-h
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 1/2] python: add bindings for notmuch_database_get_config{, _list}

---
 bindings/python/docs/source/database.rst |  4 ++
 bindings/python/notmuch/database.py      | 85 ++++++++++++++++++++++++++++++++
 bindings/python/notmuch/globals.py       |  5 ++
 3 files changed, 94 insertions(+)


_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

0001-python-add-bindings-for-notmuch_database_get_config-.patch (5K) Download Attachment
l-m-h l-m-h
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 2/2] python: add convenience function to get named queries

---
 bindings/python/notmuch/database.py | 7 +++++++
 1 file changed, 7 insertions(+)


_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

0002-python-add-convenience-function-to-get-named-queries.patch (679 bytes) Download Attachment
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/2] python: add bindings for notmuch_database_get_config{, _list}

In reply to this post by l-m-h
[hidden email] writes:


Thanks for writing these bindings, it will be good to have the bindings
(almost) catch up to the library again.


We generally expect more than just a subject line in the commit message

   https://notmuchmail.org/contributing/#index5h2

> +    def get_config(self, key):
> +        """Return the value of the given config key.

I guess we will eventually want set_config as well, even if it's not
needed for your immediate application. It might save future confusion to
add them both at the same time (unless there's something complicated
about adding set_config).

It would be good to add a couple tests. test/T590-libconfig.sh has some
C tests. I think the first one, labelled
"notmuch_database_{set,get}_config" could just be translated into python
(maybe even replace the C test with the python one, depending what
others think).

> +    def get_config_list(self, prefix):

I don't object to the simplified interface, but I would like to know
what we can do if it becomes a performance bottleneck.  Would it be
possible to replace building the list with a generator (yield statement)
without changing client code? or should we take the leap now?
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 2/2] python: add convenience function to get named queries

In reply to this post by l-m-h
[hidden email] writes:

see above re: commit messages.

> ---
>  bindings/python/notmuch/database.py | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> +
> +    def get_all_named_queries(self):
> +        """Returns a dict of all named queries mapped to their search queries.
> +
> +        This function is a python extension and not in the underlying C API.
> +        """
> +        return {k[6:]: v for k, v in self.get_config_list('query.')}

I have somewhat mixed feelings about this. I don't really like the
python bindings diverging from the C library.  It's also not clear it's
worth supporting a new API entry (since e.g. if this goes in it also
needs a test) to save the python client one line of code. On the
positive side I can see there is arguably a missing abstraction on the
library side, as those particular config items are special.
_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
l-m-h l-m-h
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/2] python: add bindings for notmuch_database_get_config{, _list}

In reply to this post by David Bremner-2
Now I finally found some time to come back to this.

Quoting David Bremner (2017-06-10 13:10:13)
> Thanks for writing these bindings, it will be good to have the bindings
> (almost) catch up to the library again.

My pleasure.

> We generally expect more than just a subject line in the commit message

OK.

> I guess we will eventually want set_config as well, even if it's not
> needed for your immediate application. It might save future confusion to
> add them both at the same time (unless there's something complicated
> about adding set_config).

Done, will send it out the next time I send the patches.

> It would be good to add a couple tests. test/T590-libconfig.sh has some
> C tests. I think the first one, labelled
> "notmuch_database_{set,get}_config" could just be translated into python
> (maybe even replace the C test with the python one, depending what
> others think).

Started it, will also come with the next round.  I would not remove the
C tests.  It could always happen that something is broken with the
python bindings even though the C bindings are fine.

> > +    def get_config_list(self, prefix):
>
> I don't object to the simplified interface, but I would like to know
> what we can do if it becomes a performance bottleneck.  Would it be
> possible to replace building the list with a generator (yield statement)
> without changing client code? or should we take the leap now?

I don't see a reason to have python programmers handle "manual
iterators" or however you want to call the thing the C code does there.
So I would like to keep *some* simplified interface as well.

It is very easy to turn this into a generator.  But then I consider the
name a mismatch.  If it is called "get_config_list" it should return a
list.  I could add get_config_iterator or get_config_generator and turn
get_config_list into a wrapper:

def get_config_list(self, prefix):
    return list(self.get_config_iterator(prefix))

The only problem one could see with this additional entry point is what
you said in id:[hidden email] about the function
get_all_named_queries (quote below), namely that the names of the python
bindings diverge from the names of the C bindings.  I don't think that
there will be a performance problem as I assume that there will not be
many config values in the database so storing them in memory should not
be a problem.

Quoting David Bremner (Message-ID: <[hidden email]>)
> I have somewhat mixed feelings about this. I don't really like the
> python bindings diverging from the C library.  It's also not clear it's
> worth supporting a new API entry (since e.g. if this goes in it also
> needs a test) to save the python client one line of code. On the
> positive side I can see there is arguably a missing abstraction on the
> library side, as those particular config items are special.

I think I will drop this commit
(34d9febc53775a24ca9e1bb1abcef64ea9196b12).

If we want to introduce some more abstract interface in python we could
also do this:

def get_configs(self, prefix):
    return dict(self.get_config_iterator(prefix))

(or with a different name like get_config_dict)

Which interface would you prefer?

Lucas

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch

signature.asc (499 bytes) Download Attachment
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/2] python: add bindings for notmuch_database_get_config{, _list}

Lucas Hoffmann <[hidden email]> writes:

>
> I don't see a reason to have python programmers handle "manual
> iterators" or however you want to call the thing the C code does there.
> So I would like to keep *some* simplified interface as well.
>
> It is very easy to turn this into a generator.  But then I consider the
> name a mismatch.  If it is called "get_config_list" it should return a
> list.  I could add get_config_iterator or get_config_generator and turn
> get_config_list into a wrapper:

For starters I'd just create get_config_{iterator,generator}, which ever
is a more pythonic name. As you point out below, it's easy to turn that
into a list or a dictionary.
>
> The only problem one could see with this additional entry point is what
> you said in id:[hidden email] about the function
> get_all_named_queries (quote below), namely that the names of the python
> bindings diverge from the names of the C bindings.

It's not so much the names diverging that I worry about as the
functionality. The C library provides (effectively) a generator, so the
bindings should too.

d

_______________________________________________
notmuch mailing list
[hidden email]
https://notmuchmail.org/mailman/listinfo/notmuch
Loading...