On Mon 24 Mar 2025 at 10:23 +0100, Michael J Gruber wrote:

> Am Mo., 24. März 2025 um 07:59 Uhr schrieb Jakub Wilk <jwilk@jwilk.net>:
>>
>> Before:
>>
>>     >>> notmuch2.Database.default_path('/dev/null')
>>     Traceback (most recent call last):
>>     ...
>>     KeyError: 'No database.path setting in /dev/null'
>>
>> After:
>>
>>     >>> notmuch2.Database.default_path('/dev/null')
>>     Traceback (most recent call last):
>>     ...
>>     notmuch2.NotmuchError: No database.path setting in /dev/null
>> ---
>
> This commit message does not explain what is to be fixed here - after
> all, `KeyError` makes perfect sense when a key is not found in (the
> non-existing) config.
>
>>  bindings/python-cffi/notmuch2/_database.py | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/bindings/python-cffi/notmuch2/_database.py b/bindings/python-cffi/notmuch2/_database.py
>> index d7485b4d..6a2b4ca1 100644
>> --- a/bindings/python-cffi/notmuch2/_database.py
>> +++ b/bindings/python-cffi/notmuch2/_database.py
>> @@ -245,7 +245,7 @@ class Database(base.NotmuchObject):
>>          try:
>>              return pathlib.Path(parser.get('database', 'path'))
>>          except configparser.Error:
>> -            raise errors.NotmuchError(
>> +            raise errors.NotmuchError(None,
>>                  'No database.path setting in {}'.format(cfg_path))
>>
>>      def __del__(self):
>> --
>
> `NotmuchError__init__(self, status=None, message=None)` is the
> relevant signature, and `NotmuchError.__new__()` uses `status` as a
> key to lookup an exception type. So the bug is that a message string
> is passed in as the first positional argument (status).
>
> A proper fix should not create the same pitfall - at least, use a
> keyword argument `message = 'No database.path setting in
> {}'.format(cfg_path)'` and leave out the positional default None. This
> is more robust, and it would also have explained right away what's
> going on.

I agree with this, using a keyword argument here would be much better.

> But do we want the generic NotmuchError here, or rather NoConfigError
> or something else?

This is a good question, IIRC the named subclasses like NoConfigError
are supposed to match directly the C-libnotmuch status codes and thus
originate from the C code.  This error is generated in the python code
however which is probably why the baseclass was chosen.  I honestly
don't know which would be more correct.

> Also, the release of notmuch 0.39 "forced" some projects to switch to
> the new bindings, and I suggest this change of error to catch (be it
> NotmuchError or something more specific) to be put in the NEWS for the
> next release right away because it breaks expections (huh!).

Yeah, sadly fixing this is also a breaking change.

Cheers,
Floris
_______________________________________________
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-leave@notmuchmail.org