Re: [PATCH 1/9] python: add a .gitignore file and refine the toplevel one

[Sebastian Spaeth <Sebastian@SSpaeth.de>]

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

On Mon, 26 Sep 2011 03:05:29 +0200, Justus Winter wrote:

#1) APPLIED
#2) APPLIED
#3) reorder the arguments of NotmuchError.__init__(): NOT APPLIED

The python tutorial gives an example of custom TransitionError with
three arguments, a custom message as the third. In addition, a STATUS
value is always expected to be given, while the additional explanatory
msg is optional, so STATUS makes for a more logical 1st parameter IMHO.
Even if it were the case, it makes for lots of code churn, longer code
(status=foo) to all Exceptions, and existing third party code would be
broken. Overall, I think there is more potential for trouble than
cleanup.

#4) APPLIED
status is always expected to be existing, but bullet proofing never hurts,
so this patch makes sense.

#5&#6) APPLIED
Modified the patches to apply again, as some changes had been made.

#7) NOT APPLIED, INPUT SOUGHT :)
I do see the value of more fine grained exceptions, but I am not sure,
we need this level of fine-grainedness. It would also make things more
tricky (the API is still actively evolving, and e.g. 4 days ago, a new
error status was added), so users of the bindings would now have
 +    NotmuchError,
 +    OutOfMemoryError,
 +    ReadOnlyDatabaseError,
 +    XapianError,
 +    FileError,
 +    FileNotEmailError,
 +    DuplicateMessageIdError,
 +    NullPointerError,
 +    TagTooLongError,
 +    UnbalancedFreezeThawError,
 +    UnbalancedAtomicError,
 +    NotInitializedError

to check where e.g. Xapian could also hide an Out of Memory. Do people
really want to catch, say UnbalancedAtomic errors specifically, rather
than testing whether an operation succeeded, and check the status code
if not? I could see the case for NotInitializedError, as that is a bit
specific to the python bindings and users might want to catch it separately.

Also, not all "status" are an error, e.g. DuplicateMessageId denotes
success rather than failure, it just communicates a status.

What do people (&user of the bindings) think would make sense here?
I am not opposed, but want more discussion and input before such a
change is made.

#8) Not merged, as it depends on #7

#9) APPLIED

Thanks for the patches, most of them are quite nice. For 7&8, I'd like
to hear more opinions.

Sebastian

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