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

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

Date: Tue, 20 Jun 2017 22:06:26 +0200

To: l-m-h@web.de, notmuch@notmuchmail.org

Cc:

From: Lucas Hoffmann


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:87wp8kcbvg.fsf@tethera.net 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: <87wp8kcbvg.fsf@tethera.net>)
> 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
signature.asc (application/pgp-signature)

Thread: