Repository: datalad/datalad-next

Issue:
Double check and document (if needed) `.name` conventions for `iter_collections`
I cannot put my finger on it, but I feel the nature of the value of `.name` for a reported item is non-uniform.

I am not aware of a documentation for the convention, if how iterators should behave.

The `.name` should always be a `PurePath|Path` objects.

However, should it carry the base path (key argument to the iterator) in it? Or not?

It would make sense to me to never have it. Rational:

There is no reason while a (file) collection should be on a file system. It could have a very non-Path base address and no hierarchical organization inside (think object store: some URL, plus some object ID).

We need to go through and verify that we did not overfit to a filesystem use case in an implementation.

Discussion:

Patch:
diff --git a/datalad_next/commands/ls_file_collection.py b/datalad_next/commands/ls_file_collection.py
index 9748cbf..5b1f4b9 100644
--- a/datalad_next/commands/ls_file_collection.py
+++ b/datalad_next/commands/ls_file_collection.py
@@ -57,7 +57,6 @@ from datalad_next.iter_collections.utils import (
 )
 from datalad_next.iter_collections.gittree import (
     GitTreeItemType,
-    GitTreeItem,
     iter_gittree,
 )
 from datalad_next.iter_collections.gitworktree import (
diff --git a/datalad_next/iter_collections/annexworktree.py b/datalad_next/iter_collections/annexworktree.py
index c3f2b61..b8943eb 100644
--- a/datalad_next/iter_collections/annexworktree.py
+++ b/datalad_next/iter_collections/annexworktree.py
@@ -134,6 +134,8 @@ def iter_annexworktree(
     Yields
     ------
     :class:`AnnexWorktreeItem` or :class:`AnnexWorktreeFileSystemItem`
+      The ``name`` attribute of an item is a ``PurePath`` instance with
+      the corresponding (relative) path, in platform conventions.
     """
 
     glsf = iter_gitworktree(
diff --git a/datalad_next/iter_collections/directory.py b/datalad_next/iter_collections/directory.py
index 027a478..06ee144 100644
--- a/datalad_next/iter_collections/directory.py
+++ b/datalad_next/iter_collections/directory.py
@@ -47,6 +47,11 @@ def iter_dir(
     Yields
     ------
     :class:`DirectoryItem`
+      The ``name`` attribute of an item is a ``Path`` instance, with the
+      format matching the main ``path`` argument. When an absolute ``path``
+      is given, item names are absolute paths too. When a relative path is
+      given, it is relative to CWD, and items names are relative paths
+      (relative to CWD) too.
     """
     for c in path.iterdir():
         # c could disappear while this is running. Example: temp files managed
diff --git a/datalad_next/iter_collections/gittree.py b/datalad_next/iter_collections/gittree.py
index 9f1a467..784e41b 100644
--- a/datalad_next/iter_collections/gittree.py
+++ b/datalad_next/iter_collections/gittree.py
@@ -6,10 +6,10 @@ from __future__ import annotations
 
 from dataclasses import dataclass
 from enum import Enum
+from functools import cached_property
 import logging
 from pathlib import (
     Path,
-    PurePath,
     PurePosixPath,
 )
 from typing import Generator
@@ -38,13 +38,20 @@ class GitTreeItemType(Enum):
 
 @dataclass
 class GitTreeItem(PathBasedItem):
-    name: PurePath
+    """``PathBasedItem`` with a relative path as a name (in POSIX conventions)
+    """
+    name: str
     # gitsha is not the sha1 of the file content, but the output
     # of `git hash-object` which does something like
     # `printf "blob $(wc -c < "$file_name")\0$(cat "$file_name")" | sha1sum`
     gitsha: str | None = None
     gittype: GitTreeItemType | None = None
 
+    @cached_property
+    def path(self) -> PurePosixPath:
+        """Returns the item name as a ``PurePosixPath`` instance"""
+        return PurePosixPath(self.name)
+
 
 _mode_type_map = {
     '100644': GitTreeItemType.file,
@@ -80,6 +87,8 @@ def iter_gittree(
     Yields
     ------
     :class:`GitTreeItem`
+      The ``name`` attribute of an item is a ``str`` with the corresponding
+      (relative) path, as reported by Git (in POSIX conventions).
     """
     # we force-convert to Path to give us the piece of mind we want.
     # The docs already ask for that, but it is easy to
@@ -111,7 +120,7 @@ def _get_tree_item(spec: str) -> GitTreeItem:
     # provides more detail
     mode, sha = props.split(' ')[0::2]
     return GitTreeItem(
-        name=PurePosixPath(path),
+        name=path,
         gitsha=sha,
         gittype=_mode_type_map[mode],
     )
diff --git a/datalad_next/iter_collections/gitworktree.py b/datalad_next/iter_collections/gitworktree.py
index 9e5993f..3d41417 100644
--- a/datalad_next/iter_collections/gitworktree.py
+++ b/datalad_next/iter_collections/gitworktree.py
@@ -43,11 +43,12 @@ lgr = logging.getLogger('datalad.ext.next.iter_collections.gitworktree')
 
 @dataclass
 class GitWorktreeItem(GitTreeItem):
-    pass
+    name: PurePath
 
 
 @dataclass
 class GitWorktreeFileSystemItem(FileSystemItem):
+    name: PurePath
     # gitsha is not the sha1 of the file content, but the output
     # of `git hash-object` which does something like
     # `printf "blob $(wc -c < "$file_name")\0$(cat "$file_name")" | sha1sum`
@@ -133,6 +134,8 @@ def iter_gitworktree(
     Yields
     ------
     :class:`GitWorktreeItem` or :class:`GitWorktreeFileSystemItem`
+      The ``name`` attribute of an item is a ``PurePath`` instance with
+      the corresponding (relative) path, in platform conventions.
     """
     # we force-convert to Path to prevent delayed crashing when reading from
     # the file system. The docs already ask for that, but it is easy to
diff --git a/datalad_next/iter_collections/tarfile.py b/datalad_next/iter_collections/tarfile.py
index 704b883..68dcc52 100644
--- a/datalad_next/iter_collections/tarfile.py
+++ b/datalad_next/iter_collections/tarfile.py
@@ -6,6 +6,7 @@ The main functionality is provided by the :func:`iter_tar()` function.
 from __future__ import annotations
 
 from dataclasses import dataclass
+from functools import cached_property
 from pathlib import (
     Path,
     PurePosixPath,
@@ -21,15 +22,25 @@ from .utils import (
 
 @dataclass  # sadly PY3.10+ only (kw_only=True)
 class TarfileItem(FileSystemItem):
-    name: PurePosixPath
+    name: str
     """TAR uses POSIX paths as item identifiers. Not all POSIX paths can
     be represented on all (non-POSIX) file systems, therefore the item
-    name is represented in POSIX form, instead of a platform-dependent
-    ``PurePath``."""
-    link_target: PurePosixPath | None = None
+    name is represented in POSIX form, instead of in platform conventions.
+    """
+    link_target: str | None = None
     """Just as for ``name``, a link target is also reported in POSIX
     format."""
 
+    @cached_property
+    def path(self) -> PurePosixPath:
+        """Returns the item name as a ``PurePosixPath`` instance"""
+        return PurePosixPath(self.name)
+
+    @cached_property
+    def link_target_path(self) -> PurePosixPath:
+        """Returns the link_target as a ``PurePosixPath`` instance"""
+        return PurePosixPath(self.link_target)
+
 
 def iter_tar(
     path: Path,
@@ -57,6 +68,8 @@ def iter_tar(
     Yields
     ------
     :class:`TarfileItem`
+      The ``name`` attribute of an item is a ``str`` with the corresponding
+      archive member name (in POSIX conventions).
     """
     with tarfile.open(path, 'r') as tar:
         for member in tar:
@@ -69,14 +82,14 @@ def iter_tar(
                 else FileSystemItemType.hardlink if member.islnk() \
                 else FileSystemItemType.specialfile
             item = TarfileItem(
-                name=PurePosixPath(member.name),
+                name=member.name,
                 type=mtype,
                 size=member.size,
                 mode=member.mode,
                 mtime=member.mtime,
                 uid=member.uid,
                 gid=member.gid,
-                link_target=PurePosixPath(member.linkname)
+                link_target=member.linkname
                 if member.linkname else None,
             )
             if fp and mtype in (
diff --git a/datalad_next/iter_collections/utils.py b/datalad_next/iter_collections/utils.py
index 91fcdc4..8d2c152 100644
--- a/datalad_next/iter_collections/utils.py
+++ b/datalad_next/iter_collections/utils.py
@@ -4,6 +4,7 @@ from __future__ import annotations
 
 from dataclasses import dataclass
 from enum import Enum
+from functools import cached_property
 import os
 from pathlib import (
     Path,
@@ -46,10 +47,27 @@ class TypedItem:
 
 @dataclass
 class PathBasedItem(NamedItem):
-    # a path-identifier in an appropriate context.
-    # could be a filename, a relpath, or an absolute path.
-    # should match platform conventions
-    name: PurePath
+    """An item with a path as its ``name``
+
+    A dedicated (cached) property supports the conversion of the
+    native name representation into a ``PurePath`` instance.
+
+    Any argument understood by the ``PurePath`` constructor can
+    be used as ``name``, such as a a filename, a relative path, or an
+    absolute path -- in string form, as path segment sequence, or
+    a ``Path`` instance.
+
+    It is recommended to use name/path values that are relative
+    to the containing collection (directory, archive, repository, etc.).
+    """
+    @cached_property
+    def path(self) -> PurePath:
+        """Returns the item name as a ``PurePath`` instance
+
+        This default implementation assumes the ``name`` to be
+        platform path conventions.
+        """
+        return PurePath(self.name)
 
 
 @dataclass  # sadly PY3.10+ only (kw_only=True)
@@ -60,9 +78,14 @@ class FileSystemItem(PathBasedItem, TypedItem):
     mode: int | None = None
     uid: int | None = None
     gid: int | None = None
-    link_target: PurePath | None = None
+    link_target: Any | None = None
     fp: IO | None = None
 
+    @cached_property
+    def link_target_path(self) -> PurePath:
+        """Returns the link_target as a ``PurePath`` instance"""
+        return PurePath(self.link_target)
+
     @classmethod
     def from_path(
         cls,
@@ -98,7 +121,8 @@ class FileSystemItem(PathBasedItem, TypedItem):
         )
         if link_target and ctype == FileSystemItemType.symlink:
             # could be p.readlink() from PY3.9+
-            item.link_target = PurePath(os.readlink(path))
+            # but check performance difference
+            item.link_target = os.readlink(path)
         return item
 
 
diff --git a/datalad_next/iter_collections/zipfile.py b/datalad_next/iter_collections/zipfile.py
index d697feb..30fa994 100644
--- a/datalad_next/iter_collections/zipfile.py
+++ b/datalad_next/iter_collections/zipfile.py
@@ -6,6 +6,7 @@ The main functionality is provided by the :func:`iter_zip()` function.
 from __future__ import annotations
 
 import datetime
+from functools import cached_property
 import time
 import zipfile
 from dataclasses import dataclass
@@ -21,31 +22,21 @@ from .utils import (
 )
 
 
-class _ZipFileDirPath(PurePosixPath):
-    """PurePosixPath variant that appends a '/' to the str-representation
-
-    This is used by class:`ZipfileItem` to represent directory members in
-    ZIP archives, in order to streamline archive member tests via a
-    ``item.name in zipfile.ZipFile(...)`` pattern. ``ZipFile`` requires
-    directory members to be identified with a trailing slash.
-    """
-    def __str__(self) -> str:
-        super_str = super().__str__()
-        return super_str if super_str.endswith('/') else f'{super_str}/'
-
-    def __eq__(self, other):
-        if not isinstance(other, _ZipFileDirPath):
-            return False
-        return super().__eq__(other)
-
-
 @dataclass
 class ZipfileItem(FileSystemItem):
-    name: PurePosixPath
-    """ZIP uses POSIX paths as item identifiers from version 6.3.3 onwards.
-    Not all POSIX paths are legal paths on non-POSIX file systems or platforms.
-    Therefore we cannot use a platform-dependent ``PurePath``-instance to
-    address ZIP-file items, anq we use ``PurePosixPath``-instances instead."""
+    name: str
+
+    @cached_property
+    def path(self) -> PurePosixPath:
+        """Returns the item name as a ``PurePosixPath`` instance
+
+        ZIP uses POSIX paths as item identifiers from version 6.3.3 onwards.
+        Not all POSIX paths are legal paths on non-POSIX file systems or
+        platforms.  Therefore we cannot use a platform-dependent
+        ``PurePath``-instance to address ZIP-file items, anq we use
+        ``PurePosixPath``-instances instead.
+        """
+        return PurePosixPath(self.name)
 
 
 def iter_zip(
@@ -74,6 +65,8 @@ def iter_zip(
     Yields
     ------
     :class:`ZipfileItem`
+      The ``name`` attribute of an item is a ``str`` with the corresponding
+      archive member name (in POSIX conventions).
     """
     with zipfile.ZipFile(path, mode='r') as zip_file:
         for zip_info in zip_file.infolist():
@@ -88,15 +81,9 @@ def iter_zip(
 
 def _get_zipfile_item(zip_info: zipfile.ZipInfo) -> ZipfileItem:
     return ZipfileItem(
-        **(
-            dict(
-                name=_ZipFileDirPath(zip_info.filename),
-                type=FileSystemItemType.directory)
-            if zip_info.is_dir()
-            else dict(
-                name=PurePosixPath(zip_info.filename),
-                type=FileSystemItemType.file)
-        ),
+        name=zip_info.filename,
+        type=FileSystemItemType.directory if zip_info.is_dir()
+        else FileSystemItemType.file,
         size=zip_info.file_size,
         mtime=time.mktime(
             datetime.datetime(*zip_info.date_time).timetuple()


Test patch:
diff --git a/datalad_next/commands/tests/test_ls_file_collection.py b/datalad_next/commands/tests/test_ls_file_collection.py
index 8b69c05..1a3222e 100644
--- a/datalad_next/commands/tests/test_ls_file_collection.py
+++ b/datalad_next/commands/tests/test_ls_file_collection.py
@@ -40,9 +40,9 @@ def _check_archive_member_result(r, collection):
     # a collection identifier, here the tar location
     assert 'collection' in r
     assert r['collection'] == collection
-    # an item identifier, here a path of an archive member
+    # an item identifier, here a str-path of an archive member
     assert 'item' in r
-    assert isinstance(r['item'], PurePath)
+    assert isinstance(r['item'], str)
     # item type info, here some filesystem-related category
     assert 'type' in r
     assert r['type'] in ('file', 'directory', 'symlink', 'hardlink')
diff --git a/datalad_next/iter_collections/tests/test_itergittree.py b/datalad_next/iter_collections/tests/test_itergittree.py
index 217b3f3..ac6c044 100644
--- a/datalad_next/iter_collections/tests/test_itergittree.py
+++ b/datalad_next/iter_collections/tests/test_itergittree.py
@@ -35,7 +35,8 @@ def test_iter_gittree(existing_dataset, no_result_rendering):
     probe.write_text('probe')
     ds.save()
     assert any(
-        i.name == PurePosixPath(f'subdir/{probe_name}')
+        # let's query a Path instance here, to get that covered too
+        i.path == PurePosixPath(f'subdir/{probe_name}')
         and i.gitsha == expected_probe_sha
         and (
             i.gittype in (GitTreeItemType.file, GitTreeItemType.executablefile)
@@ -48,7 +49,7 @@ def test_iter_gittree(existing_dataset, no_result_rendering):
         # if we check the prior version, we do not see it (hence the
         # tree-ish passing is working
         assert not any(
-            i.name == PurePosixPath(f'subdir/{probe_name}')
+            i.path == PurePosixPath(f'subdir/{probe_name}')
             for i in iter_gittree(ds.pathobj, 'HEAD~1')
         )
 
@@ -57,11 +58,11 @@ def test_iter_gittree(existing_dataset, no_result_rendering):
     tracked_toplevel_items = list(
         iter_gittree(ds.pathobj, 'HEAD', recursive='no'))
     assert not any(
-        i.name == PurePosixPath(f'subdir/{probe_name}')
+        i.name == f'subdir/{probe_name}'
         for i in tracked_toplevel_items
     )
     assert any(
-        i.name == PurePosixPath('subdir')
+        i.name == 'subdir'
         and (True if is_crippled_fs
              else 'eb4aa65f42b90178837350571a227445b996cf90')
         and i.gittype == GitTreeItemType.directory
@@ -71,7 +72,7 @@ def test_iter_gittree(existing_dataset, no_result_rendering):
     tracked_subdir_items = list(iter_gittree(probe.parent, 'HEAD'))
     assert len(tracked_subdir_items) == 1
     probe_item = tracked_subdir_items[0]
-    assert probe_item.name == PurePosixPath(probe_name)
+    assert probe_item.name == probe_name
     assert probe_item.gitsha == expected_probe_sha
 
 
@@ -83,7 +84,7 @@ def test_name_starting_with_tab(existing_dataset, no_result_rendering):
     tabbed_name = ds.pathobj / tabbed_file_name
     tabbed_name.write_text('name of this file starts with a tab')
     ds.save()
-    iter_names = [item.name for item in iter_gittree(ds.pathobj, 'HEAD')]
+    iter_names = [item.path for item in iter_gittree(ds.pathobj, 'HEAD')]
     assert PurePosixPath(tabbed_file_name) in iter_names
 
 
diff --git a/datalad_next/iter_collections/tests/test_itertar.py b/datalad_next/iter_collections/tests/test_itertar.py
index a899fee..d90ed33 100644
--- a/datalad_next/iter_collections/tests/test_itertar.py
+++ b/datalad_next/iter_collections/tests/test_itertar.py
@@ -51,7 +51,7 @@ def test_iter_tar(sample_tar_xz):
                    'md5': 'ba1f2511fc30423bdbb183fe33f3dd0f'}
     targets = [
         TarfileItem(
-            name=PurePosixPath('test-archive'),
+            name='test-archive',
             type=FileSystemItemType.directory,
             size=0,
             mtime=1683657433,
@@ -59,16 +59,16 @@ def test_iter_tar(sample_tar_xz):
             uid=1000,
             gid=1000),
         TarfileItem(
-            name=PurePosixPath('test-archive') / '123.txt',
+            name='test-archive/123.txt',
             type=FileSystemItemType.symlink,
             size=0,
             mtime=1683657414,
             mode=511,
             uid=1000,
             gid=1000,
-            link_target=PurePosixPath('subdir') / 'onetwothree_again.txt'),
+            link_target='subdir/onetwothree_again.txt'),
         TarfileItem(
-            name=PurePosixPath('test-archive') / '123_hard.txt',
+            name='test-archive/123_hard.txt',
             type=FileSystemItemType.file,
             size=4,
             mtime=1683657364,
@@ -77,7 +77,7 @@ def test_iter_tar(sample_tar_xz):
             gid=1000,
             link_target=None),
         TarfileItem(
-            name=PurePosixPath('test-archive') / 'subdir',
+            name='test-archive/subdir',
             type=FileSystemItemType.directory,
             size=0,
             mtime=1683657400,
@@ -85,7 +85,7 @@ def test_iter_tar(sample_tar_xz):
             uid=1000,
             gid=1000),
         TarfileItem(
-            name=PurePosixPath('test-archive') / 'subdir' / 'onetwothree_again.txt',
+            name='test-archive/subdir/onetwothree_again.txt',
             type=FileSystemItemType.file,
             size=4,
             mtime=1683657400,
@@ -94,14 +94,14 @@ def test_iter_tar(sample_tar_xz):
             gid=1000,
             link_target=None),
         TarfileItem(
-            name=PurePosixPath('test-archive') / 'onetwothree.txt',
+            name='test-archive/onetwothree.txt',
             type=FileSystemItemType.hardlink,
             size=0,
             mtime=1683657364,
             mode=436,
             uid=1000,
             gid=1000,
-            link_target=PurePosixPath('test-archive') / '123_hard.txt'),
+            link_target='test-archive/123_hard.txt'),
     ]
     ires = []
     for i in iter_tar(sample_tar_xz, fp=True):
diff --git a/datalad_next/iter_collections/tests/test_iterzip.py b/datalad_next/iter_collections/tests/test_iterzip.py
index cc38faa..5ace8a5 100644
--- a/datalad_next/iter_collections/tests/test_iterzip.py
+++ b/datalad_next/iter_collections/tests/test_iterzip.py
@@ -4,7 +4,6 @@ from pathlib import PurePosixPath
 
 from ..zipfile import (
     FileSystemItemType,
-    _ZipFileDirPath,
     ZipfileItem,
     iter_zip,
 )
@@ -24,7 +23,7 @@ def sample_zip(tmp_path_factory):
 
     Layout::
 
-        test-archive
+        test-archive/
         ├── onetwothree.txt
         └── subdir/
             └── onetwothree<>again.txt
@@ -48,25 +47,25 @@ def test_iter_zip(sample_zip):
         'SHA1': 'b5dfcec4d1b6166067226fae102f7fbcf6bd1bd4',
         'md5': 'd700214df5487801e8ee23d31e60382a',
     }
-    root = PurePosixPath('test-archive')
+    root = 'test-archive'
     targets = [
         ZipfileItem(
-            name=_ZipFileDirPath(root),
+            name=f'{root}/',
             type=FileSystemItemType.directory,
             size=0,
         ),
         ZipfileItem(
-            name=root / 'onetwothree.txt',
+            name=f'{root}/onetwothree.txt',
             type=FileSystemItemType.file,
             size=8,
         ),
         ZipfileItem(
-            name=_ZipFileDirPath(root, 'subdir'),
+            name=f'{root}/subdir/',
             type=FileSystemItemType.directory,
             size=0,
         ),
         ZipfileItem(
-            name=root / 'subdir' / 'onetwothree<>again.txt',
+            name=f'{root}/subdir/onetwothree<>again.txt',
             type=FileSystemItemType.file,
             size=8,
         ),
@@ -89,15 +88,3 @@ def test_iter_zip(sample_zip):
         r.mtime = None
     for t in targets:
         assert t in ires
-
-
-def test_zip_dir_path():
-    zp1 = _ZipFileDirPath("a/b")
-    zp2 = _ZipFileDirPath("a/b")
-    zp3 = _ZipFileDirPath("a/c")
-    pp = PurePosixPath("a/b")
-
-    assert zp1.as_posix()[-1] == '/'
-    assert zp1 == zp2
-    assert zp1 != zp3
-    assert zp1 != pp