Re: [PATCH 4/4] python-cffi: switch to notmuch_database_{open,create}_with_config

Subject: Re: [PATCH 4/4] python-cffi: switch to notmuch_database_{open,create}_with_config

Date: Sun, 31 Oct 2021 21:46:00 +0200

To: David Bremner, notmuch@notmuchmail.org

Cc:

From: Tomi Ollila


On Sat, Oct 30 2021, David Bremner wrote:

> Since release 0.32, libnotmuch provides searching for database and
> configuration paths. This commit changes the python module notmuch2 to
> use those facilities.
>
> This fixes the bug reported in [1], along with a couple of the
> deprecation warnings in the python bindings.
>
> Database.default_path is deprecated, since it no longer faithfully
> reflects what libnotmuch is doing, and it is also no longer used in
> the bindings themselves.
>
> This commit choose the default of config=CONFIG.EMPTY (equivalent to
> passing "" to notmuch_database_open_with_config).  This makes the
> change upward compatible API-wise (at least as far as the test suite
> verifies), but changing the default to CONFIG.SEARCH would probably be
> more convenient for bindings users.

Generally this series looks good to me -- some suspicious newlines I see,
some (if not all) of those might be ok...

>
> [1]: id:87h7d4wp6b.fsf@tethera.net
> ---
>  bindings/python-cffi/notmuch2/_build.py    | 26 ++++---
>  bindings/python-cffi/notmuch2/_database.py | 81 ++++++++++++++++------
>  doc/man1/notmuch-config.rst                |  2 +
>  test/T055-path-config.sh                   |  5 +-
>  test/T391-python-cffi.sh                   |  8 ++-
>  5 files changed, 82 insertions(+), 40 deletions(-)
>
> diff --git a/bindings/python-cffi/notmuch2/_build.py b/bindings/python-cffi/notmuch2/_build.py
> index 24df939e..f6184b97 100644
> --- a/bindings/python-cffi/notmuch2/_build.py
> +++ b/bindings/python-cffi/notmuch2/_build.py
> @@ -103,20 +103,18 @@ ffibuilder.cdef(
>      notmuch_status_to_string (notmuch_status_t status);
>  
>      notmuch_status_t
> -    notmuch_database_create_verbose (const char *path,
> -                                     notmuch_database_t **database,
> -                                     char **error_message);
> -    notmuch_status_t
> -    notmuch_database_create (const char *path, notmuch_database_t **database);
> -    notmuch_status_t
> -    notmuch_database_open_verbose (const char *path,
> -                                   notmuch_database_mode_t mode,
> -                                   notmuch_database_t **database,
> -                                   char **error_message);
> -    notmuch_status_t
> -    notmuch_database_open (const char *path,
> -                           notmuch_database_mode_t mode,
> -                           notmuch_database_t **database);
> +    notmuch_database_create_with_config (const char *database_path,
> +                                         const char *config_path,
> +                                         const char *profile,
> +                                         notmuch_database_t **database,
> +                                         char **error_message);
> +    notmuch_status_t
> +    notmuch_database_open_with_config (const char *database_path,
> +                                       notmuch_database_mode_t mode,
> +                                       const char *config_path,
> +                                       const char *profile,
> +                                       notmuch_database_t **database,
> +                                       char **error_message);
>      notmuch_status_t
>      notmuch_database_close (notmuch_database_t *database);
>      notmuch_status_t
> diff --git a/bindings/python-cffi/notmuch2/_database.py b/bindings/python-cffi/notmuch2/_database.py
> index c1fb88eb..92bfdef2 100644
> --- a/bindings/python-cffi/notmuch2/_database.py
> +++ b/bindings/python-cffi/notmuch2/_database.py
> @@ -31,6 +31,9 @@ class Mode(enum.Enum):
>      READ_ONLY = capi.lib.NOTMUCH_DATABASE_MODE_READ_ONLY
>      READ_WRITE = capi.lib.NOTMUCH_DATABASE_MODE_READ_WRITE
>  
> +class ConfigFile(enum.Enum):
> +    EMPTY = b''
> +    SEARCH = capi.ffi.NULL
>  
>  class QuerySortOrder(enum.Enum):
>      OLDEST_FIRST = capi.lib.NOTMUCH_SORT_OLDEST_FIRST
> @@ -71,6 +74,9 @@ class Database(base.NotmuchObject):
>      :cvar EXCLUDE: Which messages to exclude from queries, ``TRUE``,
>         ``FLAG``, ``FALSE`` or ``ALL``.  See the query documentation
>         for details.
> +    :cvar CONFIG: Control loading of config file. Enumeration of
> +       ``EMPTY`` (don't load a config file), and ``SEARCH`` (search as
> +       in :ref:`config_search`)
>      :cvar AddedMessage: A namedtuple ``(msg, dup)`` used by
>         :meth:`add` as return value.
>      :cvar STR_MODE_MAP: A map mapping strings to :attr:`MODE` items.
> @@ -81,9 +87,8 @@ class Database(base.NotmuchObject):
>         still open.
>  
>      :param path: The directory of where the database is stored.  If
> -       ``None`` the location will be read from the user's
> -       configuration file, respecting the ``NOTMUCH_CONFIG``
> -       environment variable if set.
> +       ``None`` the location will be searched according to
> +       :ref:`database`
>      :type path: str, bytes, os.PathLike or pathlib.Path
>      :param mode: The mode to open the database in.  One of
>         :attr:`MODE.READ_ONLY` OR :attr:`MODE.READ_WRITE`.  For
> @@ -91,17 +96,22 @@ class Database(base.NotmuchObject):
>         :attr:`MODE.READ_ONLY` and ``rw`` for :attr:`MODE.READ_WRITE`.
>      :type mode: :attr:`MODE` or str.
>  
> +    :param config: Where to load the configuration from, if any.
> +    :type config: :attr:`CONFIG.EMPTY`, :attr:`CONFIG.SEARCH`, str, bytes, os.PathLike, pathlib.Path
> +

first one above --- in database.py in current HEAD I don't see that there
is newline before :raises lines...

>      :raises KeyError: if an unknown mode string is used.
>      :raises OSError: or subclasses if the configuration file can not
>         be opened.
>      :raises configparser.Error: or subclasses if the configuration
>         file can not be parsed.
>      :raises NotmuchError: or subclasses for other failures.
> +

IIRC usually no empty line before ending """

>      """
>  
>      MODE = Mode
>      SORT = QuerySortOrder
>      EXCLUDE = QueryExclude
> +    CONFIG = ConfigFile
>      AddedMessage = collections.namedtuple('AddedMessage', ['msg', 'dup'])
>      _db_p = base.MemoryPointer()
>      STR_MODE_MAP = {
> @@ -109,18 +119,40 @@ class Database(base.NotmuchObject):
>          'rw': MODE.READ_WRITE,
>      }
>  
> -    def __init__(self, path=None, mode=MODE.READ_ONLY):
> +    @staticmethod
> +    def _cfg_path_encode(path):
> +        if isinstance(path,ConfigFile):
> +            path = path.value
> +        elif path is None:
> +            path = capi.ffi.NULL
> +        elif not hasattr(os, 'PathLike') and isinstance(path, pathlib.Path):
> +            path = bytes(path)
> +        else:
> +            path = os.fsencode(path)
> +        return path
> +
> +    @staticmethod
> +    def _db_path_encode(path):
> +        if path is None:
> +            path = capi.ffi.NULL
> +        elif not hasattr(os, 'PathLike') and isinstance(path, pathlib.Path):
> +            path = bytes(path)
> +        else:
> +            path = os.fsencode(path)
> +        return path
> +
> +    def __init__(self, path=None, mode=MODE.READ_ONLY, config=CONFIG.EMPTY):
>          if isinstance(mode, str):
>              mode = self.STR_MODE_MAP[mode]
>          self.mode = mode
> -        if path is None:
> -            path = self.default_path()
> -        if not hasattr(os, 'PathLike') and isinstance(path, pathlib.Path):
> -            path = bytes(path)
> +

hard to say above -- I might have done the same ;D

>          db_pp = capi.ffi.new('notmuch_database_t **')
>          cmsg = capi.ffi.new('char**')
> -        ret = capi.lib.notmuch_database_open_verbose(os.fsencode(path),
> -                                                     mode.value, db_pp, cmsg)
> +        ret = capi.lib.notmuch_database_open_with_config(self._db_path_encode(path),
> +                                                         mode.value,
> +                                                         self._cfg_path_encode(config),
> +                                                         capi.ffi.NULL,
> +                                                         db_pp, cmsg)
>          if cmsg[0]:
>              msg = capi.ffi.string(cmsg[0]).decode(errors='replace')
>              capi.lib.free(cmsg[0])
> @@ -132,18 +164,20 @@ class Database(base.NotmuchObject):
>          self.closed = False
>  
>      @classmethod
> -    def create(cls, path=None):
> +    def create(cls, path=None, config=ConfigFile.EMPTY):
>          """Create and open database in READ_WRITE mode.
>  
>          This is creates a new notmuch database and returns an opened
>          instance in :attr:`MODE.READ_WRITE` mode.
>  
> -        :param path: The directory of where the database is stored.  If
> -           ``None`` the location will be read from the user's
> -           configuration file, respecting the ``NOTMUCH_CONFIG``
> -           environment variable if set.
> +        :param path: The directory of where the database is stored.
> +           If ``None`` the location will be read searched by the
> +           notmuch library (see notmuch(3)::notmuch_open_with_config).
>          :type path: str, bytes or os.PathLike
>  
> +        :param config: The pathname of the notmuch configuration file.
> +        :type config: :attr:`CONFIG.EMPTY`, :attr:`CONFIG.SEARCH`, str, bytes, os.PathLike, pathlib.Path
> +

Nw that I look this the same amount of newlines as it used to be ...

>          :raises OSError: or subclasses if the configuration file can not
>             be opened.
>          :raises configparser.Error: or subclasses if the configuration
> @@ -153,15 +187,15 @@ class Database(base.NotmuchObject):
>          :raises FileError: if the database already exists.
>  
>          :returns: The newly created instance.
> +

...but here clearly added newline.

>          """
> -        if path is None:
> -            path = cls.default_path()
> -        if not hasattr(os, 'PathLike') and isinstance(path, pathlib.Path):
> -            path = bytes(path)
> +
>          db_pp = capi.ffi.new('notmuch_database_t **')
>          cmsg = capi.ffi.new('char**')
> -        ret = capi.lib.notmuch_database_create_verbose(os.fsencode(path),
> -                                                       db_pp, cmsg)
> +        ret = capi.lib.notmuch_database_create_with_config(cls._db_path_encode(path),
> +                                                           cls._cfg_path_encode(config),
> +                                                           capi.ffi.NULL,
> +                                                           db_pp, cmsg)
>          if cmsg[0]:
>              msg = capi.ffi.string(cmsg[0]).decode(errors='replace')
>              capi.lib.free(cmsg[0])
> @@ -176,7 +210,7 @@ class Database(base.NotmuchObject):
>          ret = capi.lib.notmuch_database_destroy(db_pp[0])
>          if ret != capi.lib.NOTMUCH_STATUS_SUCCESS:
>              raise errors.NotmuchError(ret)
> -        return cls(path, cls.MODE.READ_WRITE)
> +        return cls(path, cls.MODE.READ_WRITE, config=config)
>  
>      @staticmethod
>      def default_path(cfg_path=None):
> @@ -200,6 +234,9 @@ class Database(base.NotmuchObject):
>             file can not be parsed.
>          :raises NotmuchError: if the config file does not have the
>             database.path setting.
> +
> +        .. deprecated:: 0.35
> +           Use the ``cfg_path`` parameter instead.
>          """
>          if not cfg_path:
>              cfg_path = _config_pathname()
> diff --git a/doc/man1/notmuch-config.rst b/doc/man1/notmuch-config.rst
> index 7d901758..36e57ea6 100644
> --- a/doc/man1/notmuch-config.rst
> +++ b/doc/man1/notmuch-config.rst
> @@ -259,6 +259,8 @@ paths are presumed relative to `$HOME` for items in section
>  FILES
>  =====
>  
> +.. _config_search:
> +
>  CONFIGURATION
>  -------------
>  
> diff --git a/test/T055-path-config.sh b/test/T055-path-config.sh
> index d6494b92..6d9fb402 100755
> --- a/test/T055-path-config.sh
> +++ b/test/T055-path-config.sh
> @@ -309,11 +309,10 @@ EOF
>  	   ;&
>         split)
>  	   test_begin_subtest "'to' header does not crash (python-cffi) ($config)"
> -	   test_subtest_known_broken
>  	   echo 'notmuch@notmuchmail.org' > EXPECTED
>  	   test_python <<EOF
> -import notmuch2
> -db=notmuch2.Database()
> +from notmuch2 import Database
> +db=Database(config=Database.CONFIG.SEARCH)
>  m=db.find('20091117232137.GA7669@griffis1.net')
>  to=m.header('To')
>  print(to)
> diff --git a/test/T391-python-cffi.sh b/test/T391-python-cffi.sh
> index d54bad27..30872af0 100755
> --- a/test/T391-python-cffi.sh
> +++ b/test/T391-python-cffi.sh
> @@ -7,8 +7,14 @@ if [ $NOTMUCH_HAVE_PYTHON3_CFFI -eq 0 -o $NOTMUCH_HAVE_PYTHON3_PYTEST -eq 0 ]; t
>  fi
>  
>  
> -test_begin_subtest "python cffi tests"
> +test_begin_subtest "python cffi tests (NOTMUCH_CONFIG set)"
>  pytest_dir=$NOTMUCH_BUILDDIR/bindings/python-cffi/build/stage
>  printf "[pytest]\nminversion = 3.0\naddopts = -ra\n" > $pytest_dir/pytest.ini
>  test_expect_success "(cd $pytest_dir && ${NOTMUCH_PYTHON} -m pytest --verbose --log-file=$TMP_DIRECTORY/test.output)"
> +
> +test_begin_subtest "python cffi tests (NOTMUCH_CONFIG unset)"
> +pytest_dir=$NOTMUCH_BUILDDIR/bindings/python-cffi/build/stage
> +printf "[pytest]\nminversion = 3.0\naddopts = -ra\n" > $pytest_dir/pytest.ini
> +unset NOTMUCH_CONFIG
> +test_expect_success "(cd $pytest_dir && ${NOTMUCH_PYTHON} -m pytest --verbose --log-file=$TMP_DIRECTORY/test.output)"
>  test_done
> -- 
> 2.33.0
_______________________________________________
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-leave@notmuchmail.org

Thread: