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

[Lucas Hoffmann <l-m-h@web.de>]

[-- Attachment #1: Type: text/plain, Size: 3273 bytes --]

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

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

-----BEGIN PGP SIGNATURE-----

iQEzBAABCAAdFiEE96z4mcwnF18GSMtNYLcfxwAaQaEFAllJgEIACgkQYLcfxwAa
QaHRYQgApqtnjjTvA2hcu3xfvd+tKGTXv8KDn8yCfWcgrnM92cBJv9oUg0vM8q62
ZC4yZfaw3WzlxubpCLM9goE6rr3yqLs99mHUxPb1bj8P+xoJXSSKfR07uT54zvhZ
vmSTV884duRNo8pVeb5JQuwXEDROqP/R3jAkbBz9OGtjggQZJuMQwfXBPj8I7F8s
rPCebTsdUs41ksYUSuVzBwh2qjL77IRAWZplhKYOXJJxyLjTNoLcYUZ7c/9H3Xkv
EA3cGEv8r8uhw/FxkwuH4Qa4SDLQ22MHtTvlCSQZ4+ecDKkoXuWCyYnZoWPy7Ol/
EgFc8VKpN+plv0Vrt265tjTbNrZ3gQ==
=D00Y
-----END PGP SIGNATURE-----