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

classic Classic list List threaded Threaded
18 messages Options
l-m-h l-m-h
Reply | Threaded
Open this post in threaded view
|

[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
|

[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
|

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
|

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
|

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
|

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
l-m-h l-m-h
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/6] python: add bindings for notmuch_database_get_config{, _list}

Comming back after a long time (sorry for the wait).

I now changed the binding for notmuch_database_get_config_list into a
generator.  It is called get_configs in the python bindings (the "s"
should indicate the iterable/generator nature like for dict.items or
dict.keys).

Tests and the set_config entry point were also added.

If you want you can merge it as is or I can squash the commits in any
way you want.

Lucas Hoffmann (6):
  python: add bindings to access config
  python: add default arg to get_config_list
  python: turn get_config_list into a generator
  test: Add tests for new python bindings
  python: Rename get_config_list to get_configs
  test: Add test to unset config items with the python bindings

 bindings/python/docs/source/database.rst |   6 ++
 bindings/python/notmuch/database.py      | 111 ++++++++++++++++++++++++++++++-
 bindings/python/notmuch/globals.py       |   5 ++
 test/T390-python.sh                      |  81 ++++++++++++++++++++++
 4 files changed, 202 insertions(+), 1 deletion(-)

--
2.15.1

_______________________________________________
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
|

[PATCH 1/6] python: add bindings to access config


The C functions notmuch_database_get_config,
notmuch_database_get_config_list and notmuch_database_set_config are
part of the official C bindings.  So there should also be some python
bindings for them.

Also they are the only way to access the named queries introduced in
b9bf3f44.

The interface of the python functions is designed to be close to the C
functions.
---
 bindings/python/docs/source/database.rst |   6 ++
 bindings/python/notmuch/database.py      | 106 +++++++++++++++++++++++++++++++
 bindings/python/notmuch/globals.py       |   5 ++
 3 files changed, 117 insertions(+)


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

0001-python-add-bindings-to-access-config.patch (5K) Download Attachment
l-m-h l-m-h
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/6] python: add default arg to get_config_list

In reply to this post by l-m-h

It makes the function a little more intuitive to use and does not
diverge much from the original function signature.

Also an example is added to the docstring.
---
 bindings/python/notmuch/database.py | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)


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

0002-python-add-default-arg-to-get_config_list.patch (1K) Download Attachment
l-m-h l-m-h
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/6] python: turn get_config_list into a generator

In reply to this post by l-m-h

This mimics the behaviour of the underlying C function more closely as
it also does not store all values in memory.
---
 bindings/python/notmuch/database.py | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)


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

0003-python-turn-get_config_list-into-a-generator.patch (2K) Download Attachment
l-m-h l-m-h
Reply | Threaded
Open this post in threaded view
|

[PATCH 4/6] test: Add tests for new python bindings

In reply to this post by l-m-h

The tests where adopted from the tests for the corresponding C functions
in test/T590-libconfig.sh.
---
 test/T390-python.sh | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)


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

0004-test-Add-tests-for-new-python-bindings.patch (2K) Download Attachment
l-m-h l-m-h
Reply | Threaded
Open this post in threaded view
|

[PATCH 5/6] python: Rename get_config_list to get_configs

In reply to this post by l-m-h

The old name has a bit of a feeling of hungarian notation.  Also many
generators in the core are named with the suffix "s" to indicate
iterables: dict.items, dict.keys for example.
---
 bindings/python/notmuch/database.py | 18 ++----------------
 test/T390-python.sh                 | 12 ++++++------
 2 files changed, 8 insertions(+), 22 deletions(-)


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

0005-python-Rename-get_config_list-to-get_configs.patch (3K) Download Attachment
l-m-h l-m-h
Reply | Threaded
Open this post in threaded view
|

[PATCH 6/6] test: Add test to unset config items with the python bindings

In reply to this post by l-m-h
---
 test/T390-python.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)


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

0006-test-Add-test-to-unset-config-items-with-the-python-.patch (652 bytes) Download Attachment
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/6] python: add bindings for notmuch_database_get_config{, _list}

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

> Comming back after a long time (sorry for the wait).
>
> I now changed the binding for notmuch_database_get_config_list into a
> generator.  It is called get_configs in the python bindings (the "s"
> should indicate the iterable/generator nature like for dict.items or
> dict.keys).

Series pushed to master. Thanks for contributing to notmuch.

d
_______________________________________________
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
|

Re: [PATCH 5/6] python: Rename get_config_list to get_configs

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

> The old name has a bit of a feeling of hungarian notation.  Also many
> generators in the core are named with the suffix "s" to indicate
> iterables: dict.items, dict.keys for example.

I think you also need to update

  docs/source/database.rst

At least, that's my interpretation from the sphinx messages I get when I
run

% cd docs && make html

d
_______________________________________________
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
|

[PATCH 0/1] Rename get_config_list to get_configs


Sorry I overlooked that. Patch coming right up.

Lucas Hoffmann (1):
  python: Fix method name in docs

 bindings/python/docs/source/database.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.15.1

_______________________________________________
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
|

[PATCH 1/1] python: Fix method name in docs


Fix a method rename in the docs that was overlooked in
3444c731d27fd42bbbdaae00af6ca48b4525b03b.
---
 bindings/python/docs/source/database.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


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

0001-python-Fix-method-name-in-docs.patch (392 bytes) Download Attachment
David Bremner-2 David Bremner-2
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] python: Fix method name in docs

[hidden email] writes:

> Fix a method rename in the docs that was overlooked in
> 3444c731d27fd42bbbdaae00af6ca48b4525b03b.

thanks, pushed to master,

d

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