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 _______________________________________________ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch