Re: status of the new python bindings

[Floris Bruynooghe <flub@devork.be>]

Hi Anton,

Great that you're looking at this API!  My apologies for the late
response, this slipped by me probably as I was bulk marking things as
read when I came back from a few weeks away.

On Thu 07 May 2020 at 15:57 +0200, Anton Khirnov wrote:
> 1) What is the logic behind choosing whether something is exported as
> a property or as a method? E.g. Database.needs_upgrade is a property,
> while Database.revision() is a method. In my own python code, I tend to
> use @property for things that are "cheap" - i.e. do not involve
> (significant) IO or heavy computation and methods for those that do. But
> both of the above attributes involve library calls, presumably(?) of
> similar complexity. Would be nice if this was consistent.

Choosing when something should be a property is sometimes indeed tricky
and in general I agree you're right to expect property calls to be not
too expensive.  But I wouldn't go so far to consider every call into
libnotmuch as expensive.  I think there is also an element of how static
something is.  Usually I expect properties to behave more like
attributes, that is after all the point of them, and thus I don't expect
them to change often without explicitly manipulating them.

So that could justifies why I choose Database.version as a property
while Database.revision() is a method.  The revision changes all the
time as a side-effect of other things.  I think it also justifies
generally why tags are exposed as properties.

Database.needs_upgrade is indeed a more questionable choice, especially
given its naming which is more of a question being asked and thus
probably more suitable as a method.  It does change rarely, but does so
by interaction with another method - Database.upgrade().  So I would
agree that this perhaps wasn't the best choice and maybe that was better
as a method.

> 2) Atomic transactions are now exported as a context manager, which is
> nice and convenient for the usual use cases, but AFAIU does not have the
> same power. E.g. my tagging script does the tagging as a single atomic
> transaction and has a "dry-run" mode in which it omits the end_atomic()
> call, which is documented to throw away all the changes. This seems to
> not be possible with the new bindings.
> Would it be okay to add bindings for explicitly calling
> start/end_atomic()? Or is my approach considered invalid?

This is indeed a good usecase that I overlooked, I stopped reading at
the part where is says being and end atomic must always be used in
pairs...  But yes, we should add support for this too.  What would you
think of a .abort() on the context manager instead of directly exposing
the start/end?  E.g.:

db = notmuch2.Database()
with db.atomic() as txn:
    # do stuff
    txn.abort()

However if I read the docs correctly the transaction only actually is
aborted once the database is closed?  But when I try this:

with db.atomic():
    # do stuff
    db.close()

I discover that after calling .close() it's very easy to segfault by
making other calls, e.g. the above currently segfaults.  I thought this
wasn't so bad but perhaps .close() should immediately destroy the
database too.  This would again disallow some funky behaviour that the
raw C API allows I guess where some objects might still be valid to use.
But this is really hard to get right I think and I'd prefer to vote on
the side of safety in the Python bindings.

Still, it could be reasonable to make AtomicContext.abort() also close
the database and then make sure it doesn't call
notmuch_database_end_atomic() on exit.  I tried this locally and it
seems reasonable.  What do you think?


Lastly you can do this today by calling the C API directly:

db = notmuch2.Database()
notmuch2._capi.notmuch_database_being_atomic(db._db_p)

Yes, that uses a lot of internal stuff but it's also pretty advanced
usage.  I'm only suggesting you this to maybe unblock you in the short
term.


> 3) There seem to be no bindings for notmuch_database_set_config().

There are still bits of the API missing indeed.  Would be great to add
them but would be even better to have someone who needs it to validate
the API.  There were recently some good patches posted to expose the
config stuff, would be good to see that merged indeed.

> 4) The setup for building the documentation seems to be missing.

Hmm, yes.  The docstrings are written to work with sphinx autodoc but a
doc directory with sphinx configured never got added.  Perhaps we should
do this so that people can at least build docs locally.

> Anything else of note that remains to be implemented?

Probably.


Thanks for caring!

Cheers,
Floris