Release 0.1.23: strict push_to_dds argument checks and clearer call errors (ref #306883)

README.md

@@ -36,7 +36,7 @@ pip install "git+https://github.com/eeadata/EEALakeHouse.python.git@staging"
 ```
 
 # staging's latest release (early access) — pin to the tag the "staging" badge above shows
-pip install "git+https://github.com/eeadata/EEALakeHouse.python.git@v0.1.22-staging"
+pip install "git+https://github.com/eeadata/EEALakeHouse.python.git@v0.1.23-staging"
 ```
 
 ## Usage
@@ -428,7 +428,7 @@ To pin to one specific release instead, use the exact tag the live badges under
 %pip install "git+https://github.com/eeadata/EEALakeHouse.python.git@v0.1.22"
 
 # staging's latest release (early access)
-%pip install "git+https://github.com/eeadata/EEALakeHouse.python.git@v0.1.22-staging"
+%pip install "git+https://github.com/eeadata/EEALakeHouse.python.git@v0.1.23-staging"
 ```
 
 Use the `%pip` magic rather than `!pip` — it installs into the kernel the

pyproject.toml

@@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta"
 
 [project]
 name = "EEADataLakehouse"
-version = "0.1.22"
+version = "0.1.23"
 description = "Data preparation and validation utilities for the EEA data lakehouse pipeline."
 readme = "README.md"
 requires-python = ">=3.11"

src/eea_datalakehouse/notebook/magics.py

@@ -86,8 +86,10 @@
 
 from __future__ import annotations
 
+import ast
 import inspect
 import os
+import re
 from html import escape as _escape
 from typing import Any
 
@@ -412,6 +414,106 @@ def _print_help_table(
     display(HTML(_help_table_html(rows)))
 
 
+_CALL = re.compile(r"\s*([A-Za-z_]\w*)\s*\((.*)\)\s*", re.DOTALL)
+_CALLEE = re.compile(r"\s*([A-Za-z_]\w*)\s*\(")
+_KEYWORD_ARG = re.compile(r"\s*([A-Za-z_]\w*)\s*=(?!=)")
+
+
+def _split_args(text: str) -> list[str] | None:
+    """The top-level, comma-separated arguments of a call, as written.
+
+    Commas inside brackets or string literals don't split. ``None`` if the
+    brackets or quotes don't balance — nothing useful can be said then.
+    """
+    args: list[str] = []
+    depth = 0
+    quote: str | None = None
+    start = 0
+    i = 0
+    while i < len(text):
+        char = text[i]
+        if quote:
+            if char == "\\":
+                i += 1
+            elif char == quote:
+                quote = None
+        elif char in "\"'":
+            quote = char
+        elif char in "([{":
+            depth += 1
+        elif char in ")]}":
+            depth -= 1
+            if depth < 0:
+                return None
+        elif char == "," and depth == 0:
+            args.append(text[start:i].strip())
+            start = i + 1
+        i += 1
+    if quote or depth:
+        return None
+    last = text[start:].strip()
+    if last:
+        args.append(last)
+    return args
+
+
+def _fits(annotation: Any, value_text: str) -> bool:
+    """Whether the literal ``value_text`` could be a value for ``annotation``.
+
+    Anything that isn't a literal (a variable name, an expression) could be
+    anything, so it fits every parameter.
+    """
+    try:
+        value = ast.literal_eval(value_text)
+    except (ValueError, SyntaxError):
+        return True
+    if annotation is inspect.Parameter.empty:
+        return True
+    allowed = {part.strip() for part in str(annotation).split("|")}
+    return type(value).__name__ in allowed or (value is None and "None" in allowed)
+
+
+def _explain_syntax_error(session: Any, line: str, exc: SyntaxError) -> str:
+    """A magic line that isn't valid Python, explained against the method it calls.
+
+    For the commonest slip — an unnamed argument after a ``name=value`` one —
+    names the offending argument and suggests the parameters it could be;
+    always ends with the correct form of the call when the method is known.
+    """
+    callee = _CALLEE.match(line)
+    method = getattr(type(session), callee.group(1), None) if callee else None
+    if not callable(method):
+        return f"invalid syntax: {exc.msg}"
+    usage = f"usage: {method.__name__}({_plain_params(method)})"
+    match = _CALL.fullmatch(line)
+    args = _split_args(match.group(2)) if match else None
+    if args:
+        keywords = [_KEYWORD_ARG.match(arg) for arg in args]
+        first_keyword = next((i for i, kw in enumerate(keywords) if kw), None)
+        stray = next(
+            (i for i, kw in enumerate(keywords) if first_keyword is not None
+             and i > first_keyword and kw is None),
+            None,
+        )
+        if first_keyword is not None and stray is not None:
+            named = {kw.group(1) for kw in keywords if kw}
+            params = [
+                p for name, p in inspect.signature(method).parameters.items() if name != "self"
+            ]
+            candidates = [
+                f"{p.name}={args[stray]}"
+                for p in params[first_keyword:]
+                if p.name not in named and _fits(p.annotation, args[stray])
+            ]
+            hint = f" Did you mean {' or '.join(candidates)}?" if candidates else ""
+            return (
+                f"argument {stray + 1} ({args[stray]}) has no name, but comes after "
+                f"{args[first_keyword]} — once an argument is given by name, every argument "
+                f"after it needs a name too.{hint}\n{usage}"
+            )
+    return f"invalid syntax: {exc.msg}\n{usage}"
+
+
 _DISPATCH_FAILED = object()
 # Sentinel `_dispatch` returns when it printed a friendly error — distinct
 # from a legitimate `None` result (e.g. `use(...)` printing its own status
@@ -453,6 +555,10 @@ def _dispatch(
                 # the context they just set instead of an empty CommitReport.
                 print(f"context set to {session.get_context()!r}")
                 result = None
+    except SyntaxError as exc:
+        # The line itself isn't valid Python, so the call never happened.
+        print(f"{label} error: {_explain_syntax_error(session, line, exc)}")
+        return _DISPATCH_FAILED
     except error_type as exc:
         print(f"{label} error: {exc}")
         return _DISPATCH_FAILED

src/eea_datalakehouse/sdi/controller.py

@@ -199,6 +199,15 @@ def push_to_dds(
         The XML is re-checked first, so a hand-built :class:`SdiMetadata`
         cannot put one record under another's UUID.
         """
+        check_arg_types(
+            "push_to_dds",
+            ("metadata", metadata, SdiMetadata, False),
+            ("dds_path", dds_path, str, False),
+            ("target_name", target_name, str, True),
+            ("folder", folder, str, False),
+            ("force", force, bool, False),
+            ("check_catalog", check_catalog, bool, False),
+        )
         iso.check(metadata.xml, metadata.uuid)
         target = metadata_path(dds_path, metadata.uuid, folder=folder, target_name=target_name)
         if check_catalog and self._catalog is not None:
@@ -345,7 +354,7 @@ def to_catalog_path(path: str) -> str:
 
 def metadata_path(
     dds_path: str,
-    uuid: str,
+    uuid: str | SdiMetadata,
     *,
     folder: str = DEFAULT_FOLDER,
     target_name: str | None = None,
@@ -355,8 +364,10 @@ def metadata_path(
     ``dds_path`` may be in catalog format (``a.b.c``) or storage format
     (``a/b/c``); see :func:`to_storage_path`. ``target_name`` is the file
     name, ``{uuid}.xml`` by default; ``.xml`` is added if it has no such
-    extension.
+    extension. ``uuid`` may also be the :class:`SdiMetadata` itself.
     """
+    if isinstance(uuid, SdiMetadata):
+        uuid = uuid.uuid
     parent = to_storage_path(dds_path).strip("/")
     sub = folder.strip().strip("/").lower()
     if not parent:
@@ -368,19 +379,75 @@ def metadata_path(
     return f"{parent}/{sub}/{name}"
 
 
+_INVALID_NAME_CHARS = set('<>:"|?*')
+
+
+def check_arg_types(
+    func: str, *specs: tuple[str, object, type | tuple[type, ...], bool]
+) -> None:
+    """Raise ``TypeError`` unless every argument has exactly its declared type.
+
+    ``specs`` are ``(name, value, expected, optional)`` in the function's
+    parameter order, so the message can name the position too — the argument
+    most likely to be wrong is one passed positionally into the wrong slot.
+    Strict: ``bool`` parameters take only ``True`` / ``False`` (not ``0`` /
+    ``1``), and a ``str`` parameter takes no other type. ``optional`` allows
+    ``None``.
+    """
+    for position, (name, value, expected, optional) in enumerate(specs, start=1):
+        if value is None and optional:
+            continue
+        kinds = expected if isinstance(expected, tuple) else (expected,)
+        # bool is a subclass of int: only a bool parameter may take a bool.
+        if any(
+            isinstance(value, kind) and (kind is bool or not isinstance(value, bool))
+            for kind in kinds
+        ):
+            continue
+        wanted = " or ".join(kind.__name__ for kind in kinds) + (" or None" if optional else "")
+        hint = (
+            " — pass the record as metadata=..."
+            if isinstance(value, SdiMetadata) and name != "metadata"
+            else ""
+        )
+        raise TypeError(
+            f"{func}: argument {position} ({name}) must be {wanted}, "
+            f"not {type(value).__name__}{hint}"
+        )
+
+
 def _require_target_name(target_name: str) -> str:
-    """``target_name`` as a single file name ending in ``.xml``."""
+    """``target_name`` as a valid single file name ending in ``.xml``.
+
+    Raises ``TypeError`` unless it is a ``str`` (an :class:`SdiMetadata`
+    passed here by mistake gets a hint to use ``metadata=``), and
+    ``ValueError`` if it is empty, a path (``/``, ``\\``, ``.``, ``..``), or
+    has a character no file system accepts in a name (``< > : " | ? *`` or a
+    control character).
+    """
+    if not isinstance(target_name, str):
+        raise TypeError(
+            f"target_name must be a file name (str), not {type(target_name).__name__}"
+            + (" — pass the record as metadata=..." if isinstance(target_name, SdiMetadata) else "")
+        )
     name = target_name.strip()
     if not name:
         raise ValueError("target_name must not be empty")
     if "/" in name or "\\" in name or name in (".", ".."):
         raise ValueError(f"target_name must be a file name, not a path: {target_name!r}")
+    bad = sorted({c for c in name if c in _INVALID_NAME_CHARS or ord(c) < 32 or ord(c) == 127})
+    if bad:
+        raise ValueError(
+            f"target_name {target_name!r} is not a valid file name: it contains {bad!r}"
+        )
     if not name.lower().endswith(".xml"):
         name += ".xml"
     return name
 
 
 def _require_uuid(uuid: str) -> str:
+    if not isinstance(uuid, str):
+        raise TypeError(f"uuid must be a str, not {type(uuid).__name__}")
     uuid = uuid.strip()
     if not uuid:
         raise ValueError("uuid must not be empty")

src/eea_datalakehouse/sdi/session.py

@@ -40,6 +40,7 @@
 
 from __future__ import annotations
 
+import inspect
 import os
 from collections.abc import Callable, Mapping
 from functools import wraps
@@ -58,7 +59,14 @@
     MissingCredentialsError,
 )
 from .catalogue import SdiCatalogue
-from .controller import DEFAULT_FOLDER, PushResult, SdiController, SdiMetadata
+from .controller import (
+    DEFAULT_FOLDER,
+    PushResult,
+    SdiController,
+    SdiMetadata,
+    check_arg_types,
+    metadata_path,
+)
 from .errors import SdiError
 
 # What `%catalog` reads (see notebook/magics.py's _build_catalog_session).
@@ -70,7 +78,14 @@
 _F = TypeVar("_F", bound=Callable[..., Any])
 
 # Failures a notebook user can act on; anything else is a bug and keeps its traceback.
-_EXPECTED = (SdiError, DocumentsApiError, MissingCredentialsError, ValueError, httpx.HTTPError)
+_EXPECTED = (
+    SdiError,
+    DocumentsApiError,
+    MissingCredentialsError,
+    ValueError,
+    TypeError,
+    httpx.HTTPError,
+)
 
 
 class SdiSessionError(RuntimeError):
@@ -81,12 +96,35 @@ class MetadataSessionError(RuntimeError):
     """Any failure of a `MetadataSession` call; the original error is ``__cause__``."""
 
 
+def call_usage(method: Callable[..., Any]) -> str:
+    """``name(a, b=None, ...)`` — how to call ``method``, without ``self`` or type hints."""
+    params = [
+        name if param.default is inspect.Parameter.empty else f"{name}={param.default!r}"
+        for name, param in inspect.signature(method).parameters.items()
+        if name != "self"
+    ]
+    return f"{method.__name__}({', '.join(params)})"
+
+
 def _friendly(error: type[RuntimeError]) -> Callable[[_F], _F]:
-    """Re-raise every expected failure of the decorated method as ``error``."""
+    """Re-raise every expected failure of the decorated method as ``error``.
+
+    A call that doesn't fit the method's signature (an unknown keyword, an
+    argument given twice, too many arguments) is reported the same way, with
+    the correct form of the call.
+    """
 
     def decorate(method: _F) -> _F:
+        signature = inspect.signature(method)
+
         @wraps(method)
         def wrapper(*args: Any, **kwargs: Any) -> Any:
+            try:
+                signature.bind(*args, **kwargs)
+            except TypeError as exc:
+                raise error(
+                    f"{method.__name__}: {exc}\nusage: {call_usage(method)}"
+                ) from None
             try:
                 return method(*args, **kwargs)
             except error:
@@ -241,9 +279,22 @@ def push_to_dds(
         `{dds_path}/{folder}/{target_name}` in DDS — `{uuid}.xml` unless
         `target_name` is given — after checking `dds_path` exists in the
         Dremio catalog (skip with `check_catalog=False`)."""
+        # Arguments first, strictly typed in this method's own parameter order:
+        # a bad argument is reported as such, before any DDS or Dremio settings
+        # are looked up.
+        check_arg_types(
+            "push_to_dds",
+            ("dds_path", dds_path, str, False),
+            ("target_name", target_name, str, True),
+            ("metadata", metadata, SdiMetadata, True),
+            ("folder", folder, str, False),
+            ("force", force, bool, False),
+            ("check_catalog", check_catalog, bool, False),
+        )
         metadata = metadata or self._last()
         if metadata is None:
             raise MetadataSessionError("nothing to push yet — run %sdi get_xml(uuid) first")
+        metadata_path(dds_path, metadata.uuid, folder=folder, target_name=target_name)
         return self._push_controller(with_catalog=check_catalog).push_to_dds(
             metadata,
             dds_path,