diff --git a/packages/timenet/src/timenet/cli/app.py b/packages/timenet/src/timenet/cli/app.py index 4900abd56..8115391de 100644 --- a/packages/timenet/src/timenet/cli/app.py +++ b/packages/timenet/src/timenet/cli/app.py @@ -10,6 +10,7 @@ import typer from timenet.cache import cache_size, clear_cache, clear_releases, human_bytes, stored_releases +from timenet.cli.options import AUTO_BUILD, LOCAL_REGISTRY, REMOTE_REGISTRY from timenet.cli.runner import run_cli from timenet.cli.ui import console as ui, progress_renderer from timenet.client import TimeNet, default_local_registry, default_sources @@ -24,22 +25,6 @@ app = typer.Typer(help="Browse a TimeNet registry and fetch datasets.", no_args_is_help=True) console = Console() -_registry_option = typer.Option( - None, - "--registry", - "-r", - help="Registry URL or path that this command reads (else $TIMENET_REMOTE_REGISTRY, then timenet://).", -) -# TODO(adrian): Remove temporary glue which maps --registry onto the list after the local registry, until the CLI -# takes the flags of the defaults. -_download_registry_option = typer.Option( - None, - "--registry", - "-r", - help="Registry URL or path to put after the local registry in the list of sources. The release resolves " - "through the whole list.", -) - T = TypeVar("T") @@ -71,18 +56,21 @@ def _enum_list(values: list[str], factory: Callable[[str], T], flag: str, choice return parsed or None -def _client(registry: str | None) -> TimeNet: - """Create a client with the default sources and an optional registry. +def _client(local_registry: str | None, remote_registry: str | None, auto_build: bool | None) -> TimeNet: + """Create a client with the selected registries and build setting. + + Pass ``None`` to use the matching ``TIMENET_*`` environment variable or its default. Args: - registry: The registry URL or path to read after the local registry, or ``None`` for the default. + local_registry: The local registry path. + remote_registry: The remote registry URL or path. + auto_build: Whether to build missing datasets with installed connectors. Returns: - A client with the configured sources. + The configured client. """ - # TODO(adrian): Remove temporary glue which maps --registry onto default_sources() until the - # CLI takes the flags of the defaults. - return TimeNet(sources=default_sources(remote_registry=registry)) + sources = default_sources(local_registry=local_registry, remote_registry=remote_registry, auto_build=auto_build) + return TimeNet(sources=sources) def _metadata_table(rows: list[DatasetMetadata], title: str) -> Table: @@ -111,9 +99,13 @@ def _metadata_table(rows: list[DatasetMetadata], title: str) -> Table: @app.command("list") -def list_datasets(registry: str | None = _registry_option) -> None: - """Display metadata for every dataset in the selected registry.""" - rows = _client(registry).list() +def list_datasets( + local_registry: str | None = LOCAL_REGISTRY, + remote_registry: str | None = REMOTE_REGISTRY, + auto_build: bool | None = AUTO_BUILD, +) -> None: + """List the datasets in the registry.""" + rows = _client(local_registry, remote_registry, auto_build).list() if not rows: typer.echo("No datasets in the registry.") return @@ -122,7 +114,6 @@ def list_datasets(registry: str | None = _registry_option) -> None: @app.command() def search( # ruff: ignore[too-many-arguments, too-many-positional-arguments] - registry: str | None = _registry_option, query: list[str] = typer.Option([], "--query", "-q", help="Free-text terms."), domain: list[str] = typer.Option([], "--domain", help="Domain(s)."), task: list[str] = typer.Option([], "--task", help="Task type(s)."), @@ -131,12 +122,15 @@ def search( # ruff: ignore[too-many-arguments, too-many-positional-arguments] dataset_id: list[str] = typer.Option([], "--id", help="Dataset id(s)."), tag: list[str] = typer.Option([], "--tag", help="Tag(s)."), limit: int = typer.Option(100, "--limit", min=0, help="Maximum results."), + local_registry: str | None = LOCAL_REGISTRY, + remote_registry: str | None = REMOTE_REGISTRY, + auto_build: bool | None = AUTO_BUILD, ) -> None: - """Find datasets that match the filters. + """Search for datasets that match the filters. - Repeat a filter flag to pass several values. + Repeat a filter flag to supply multiple values. """ - results = _client(registry).search( + results = _client(local_registry, remote_registry, auto_build).search( query=query or None, domain=_enum_list(domain, Domain, "--domain", [d.value for d in Domain]), task=_enum_list(task, lambda value: TASKS[TaskType(value)], "--task", [t.value for t in TaskType]), @@ -153,9 +147,14 @@ def search( # ruff: ignore[too-many-arguments, too-many-positional-arguments] @app.command() -def info(dataset_id: str, registry: str | None = _registry_option) -> None: - """Display a dataset's metadata, schema, and counts.""" - manifest = _client(registry).get(dataset_id) +def info( + dataset_id: str, + local_registry: str | None = LOCAL_REGISTRY, + remote_registry: str | None = REMOTE_REGISTRY, + auto_build: bool | None = AUTO_BUILD, +) -> None: + """Show a dataset's metadata, schema, and counts.""" + manifest = _client(local_registry, remote_registry, auto_build).get(dataset_id) counts = manifest.counts table = Table(title=manifest.dataset_id, title_justify="left", title_style="bold", show_header=False) table.add_column("Field", style="cyan") @@ -197,24 +196,25 @@ def info(dataset_id: str, registry: str | None = _registry_option) -> None: @app.command() def download( dataset_id: str, - registry: str | None = _download_registry_option, + local_registry: str | None = LOCAL_REGISTRY, + remote_registry: str | None = REMOTE_REGISTRY, + auto_build: bool | None = AUTO_BUILD, force: bool = typer.Option( False, "--force", help="Copy the release again, even if the local registry holds a complete copy." ), ) -> None: - """Copy a release and its payload sources into the local registry. + """Copy a release and its data files into the local registry. - Print the release directory to standard output. + The command prints the release directory to stdout. """ - sources = default_sources(remote_registry=registry) started: list[StepStarted] = [] with progress_renderer() as show: - target = TimeNet(sources=sources).download( - dataset_id, force=force, progress=partial(_keep_steps, started, show) - ) + client = _client(local_registry, remote_registry, auto_build) + target = client.download(dataset_id, force=force, progress=partial(_keep_steps, started, show)) if not started: # The local registry already held the release and its payload sources. - ui.success(f"Nothing to do: '{split_ref(dataset_id)[0]}' {target.name} is already in {sources[0]}") + store = default_local_registry(local_registry=local_registry) + ui.success(f"Nothing to do: '{split_ref(dataset_id)[0]}' {target.name} is already in {store}") typer.echo(str(target)) @@ -244,9 +244,9 @@ def _keep_steps(started: list[StepStarted], show: ProgressCallback | None, event @local_app.command("info") -def local_info() -> None: - """Show the path and size of the cache and the local registry.""" - store, cache_dir = default_local_registry(), settings().cache_dir +def local_info(local_registry: str | None = LOCAL_REGISTRY) -> None: + """Show cache and local registry paths and sizes.""" + store, cache_dir = default_local_registry(local_registry=local_registry), settings().cache_dir releases = stored_releases(store) size = human_bytes(sum(item.size_bytes for item in releases)) # Plain lines, so that a long path does not wrap. @@ -255,9 +255,9 @@ def local_info() -> None: @local_app.command("clear") -def local_clear(yes: bool = _YES) -> None: +def local_clear(yes: bool = _YES, local_registry: str | None = LOCAL_REGISTRY) -> None: """Delete the cache and every release in the local registry.""" - store, cache_dir = default_local_registry(), settings().cache_dir + store, cache_dir = default_local_registry(local_registry=local_registry), settings().cache_dir if not yes: typer.confirm(f"Delete the cache in {cache_dir} and every release in {store.root}?", abort=True) # The cache clear checks the folders before it deletes anything, so it runs first. diff --git a/packages/timenet/src/timenet/cli/options.py b/packages/timenet/src/timenet/cli/options.py new file mode 100644 index 000000000..b348d08af --- /dev/null +++ b/packages/timenet/src/timenet/cli/options.py @@ -0,0 +1,29 @@ +"""CLI flags for registry locations and automatic builds. + +Each flag overrides the matching ``TIMENET_*`` environment variable. +Omitted flags have the value ``None``. +""" + +import typer + + +LOCAL_REGISTRY = typer.Option( + None, + "--local-registry", + metavar="PATH", + help="A local registry: the first source that a lookup checks, and the place where downloads and builds land " + "(default: $TIMENET_LOCAL_REGISTRY, else ~/.timenet/registry).", +) +REMOTE_REGISTRY = typer.Option( + None, + "--remote-registry", + metavar="URL|PATH", + help="A remote registry, as a URL or a local path: the second source that a lookup checks, if the local " + "registry does not have the dataset (default: $TIMENET_REMOTE_REGISTRY, else timenet://).", +) +AUTO_BUILD = typer.Option( + None, + "--auto-build/--no-auto-build", + help="Whether installed connectors build a dataset that neither the local nor the remote registry has " + "(default: $TIMENET_AUTO_BUILD, else true).", +) diff --git a/packages/timenet/src/timenet/config.py b/packages/timenet/src/timenet/config.py index b32c68b76..077278de0 100644 --- a/packages/timenet/src/timenet/config.py +++ b/packages/timenet/src/timenet/config.py @@ -32,9 +32,12 @@ ) """The cache marker text, starting with the standard cache tag signature.""" +_FLAGS = {"local_registry": "--local-registry", "remote_registry": "--remote-registry"} +"""CLI flag names for the registry settings.""" + class TimeNetSettings(BaseSettings): - """TimeNet settings from arguments, ``TIMENET_*`` variables and an optional ``.env`` file.""" + """TimeNet settings from arguments, ``TIMENET_*`` environment variables, and an optional ``.env`` file.""" model_config = SettingsConfigDict(env_prefix="TIMENET_", env_file=".env", extra="ignore") @@ -58,24 +61,25 @@ class TimeNetSettings(BaseSettings): @field_validator("local_registry", "remote_registry", "cache", mode="before") @classmethod def _not_empty(cls, value: Any, info: ValidationInfo) -> Any: - """Convert paths to strings and reject blank settings. + """Convert paths to strings and reject blank strings. Args: - value: The value to validate. - info: Validation metadata that includes the field name. + value: A setting value. + info: Details of the field to validate. Returns: - The value with any path converted to a string. + The value, with any path converted to a string. Raises: - TimeFValidationError: If the value is empty or contains only whitespace. + TimeFValidationError: If the string is empty or contains only whitespace. """ if isinstance(value, os.PathLike): value = os.fspath(value) if isinstance(value, str) and not value.strip(): name = str(info.field_name) + flag = f" (also as {_FLAGS[name]})" if name in _FLAGS else "" raise TimeFValidationError( - f"TIMENET_{name.upper()} must not be empty; give it a value, " + f"TIMENET_{name.upper()} must not be empty{flag}; give it a value, " f"or leave it unset for {cls.model_fields[name].default}." ) return value diff --git a/packages/timenet/tests/test_cli.py b/packages/timenet/tests/test_cli.py index d6af67f8b..2b32bbfed 100644 --- a/packages/timenet/tests/test_cli.py +++ b/packages/timenet/tests/test_cli.py @@ -132,30 +132,24 @@ def test_local_clear_deletes_no_release_when_the_cache_check_fails(state, monkey assert inside.exists("timenet/hello-world", "1.0.0") -def test_list(registry_root): - result = runner.invoke(app, ["list", "--registry", str(registry_root)]) +def test_the_local_commands_take_the_local_registry_flag(tmp_path): + # The variable of the autouse fixture names an empty store, so the release comes from the flag. + store = tmp_path / "store" + LocalRegistry(store).put_dataset(make_dataset()) + result = runner.invoke(app, ["local", "info", "--local-registry", str(store)]) assert result.exit_code == 0, result.output - assert "timenet/hello-world" in click.unstyle(result.output) - - -def test_search_by_domain(registry_root): - result = runner.invoke(app, ["search", "--registry", str(registry_root), "--domain", "general"]) + assert f"Local registry {store}: 1 release" in click.unstyle(result.stdout) + result = runner.invoke(app, ["local", "clear", "--yes", "--local-registry", str(store)]) assert result.exit_code == 0, result.output - assert "timenet/hello-world" in click.unstyle(result.output) + assert stored_releases(LocalRegistry(store)) == [] def test_search_no_match(registry_root): - result = runner.invoke(app, ["search", "--registry", str(registry_root), "--domain", "cardiology"]) + result = runner.invoke(app, ["search", "--remote-registry", str(registry_root), "--domain", "cardiology"]) assert result.exit_code == 0, result.output assert click.unstyle(result.output) == "No datasets match.\n" -def test_info(registry_root): - result = runner.invoke(app, ["info", "timenet/hello-world", "--registry", str(registry_root)]) - assert result.exit_code == 0, result.output - assert click.unstyle(result.output).startswith("timenet/hello-world") - - def _write(root, dataset): with TimeFWriter(root, dataset) as writer: writer.write() @@ -190,10 +184,11 @@ def test_info_lists_the_access_page_of_each_release_that_sets_one(tmp_path): child.import_from(reader) _write(root, child) - result = runner.invoke(app, ["info", "test/child", "--registry", str(root)]) + result = runner.invoke(app, ["info", "test/child", "--remote-registry", str(root)]) assert result.exit_code == 0, result.output - cells = [[cell.strip() for cell in line.split("│")[1:-1]] for line in result.output.splitlines() if "│" in line] + lines = click.unstyle(result.output).splitlines() + cells = [[cell.strip() for cell in line.split("│")[1:-1]] for line in lines if "│" in line] start = cells.index(["access", "credentialed"]) # The card of the child stays open. Its effective level comes from its parents, and each parent # names its own page. @@ -208,7 +203,7 @@ def test_download_command_copies_into_the_store_and_then_has_nothing_to_do(regis # The last line names a long path, so the console must not wrap it. monkeypatch.setattr(_cli_module.ui, "rich", Console(stderr=True, width=1000)) monkeypatch.setenv("TIMENET_LOCAL_REGISTRY", str(tmp_path / "store")) - command = ["download", "timenet/hello-world", "--registry", str(registry_root)] + command = ["download", "timenet/hello-world", "--remote-registry", str(registry_root)] target = tmp_path / "store" / "timenet/hello-world" / "1.0.0" copied = runner.invoke(app, command) @@ -229,7 +224,7 @@ def test_download_command_copies_into_the_store_and_then_has_nothing_to_do(regis def test_download_command_with_force_copies_a_held_release_again(registry_root, tmp_path, monkeypatch): monkeypatch.setenv("TIMENET_LOCAL_REGISTRY", str(tmp_path / "store")) - command = ["download", "timenet/hello-world", "--registry", str(registry_root)] + command = ["download", "timenet/hello-world", "--remote-registry", str(registry_root)] assert runner.invoke(app, command).exit_code == 0 forced = runner.invoke(app, [*command, "--force"]) @@ -239,8 +234,48 @@ def test_download_command_with_force_copies_a_held_release_again(registry_root, assert successes == ["✅ Copied 'timenet/hello-world' → 1.0.0"] +def test_each_command_builds_the_list_from_its_flags(registry_root, tmp_path): + # The variables of the autouse fixture name an empty store and an empty registry, so each result + # comes from the flags. + store = tmp_path / "store" + flags = ["--local-registry", str(store), "--remote-registry", str(registry_root), "--no-auto-build"] + for command in (["list"], ["search", "--domain", "general"], ["info", "timenet/hello-world"]): + result = runner.invoke(app, [*command, *flags]) + assert result.exit_code == 0, result.output + assert "timenet/hello-world" in result.stdout + result = runner.invoke(app, ["download", "timenet/hello-world", *flags]) + assert result.exit_code == 0, result.output + assert result.stdout == f"{store / 'timenet/hello-world/1.0.0'}\n" + + +@pytest.mark.parametrize(("flag", "variable"), [("--auto-build", "false"), ("--no-auto-build", "true")]) +def test_the_auto_build_flag_beats_its_variable(monkeypatch, flag, variable): + monkeypatch.setenv("TIMENET_AUTO_BUILD", variable) + result = runner.invoke(app, ["download", "x/y", flag]) + # The error names each source of the list. + assert isinstance(result.exception, TimeNetDatasetNotFoundError) + assert ("installed connectors" in str(result.exception)) == (flag == "--auto-build") + + +# The commands that read the list of sources share one path, so download stands for them. +_EMPTY_FLAGS = [ + ["download", "x/y", "--local-registry", ""], + ["local", "clear", "--yes", "--local-registry", ""], +] + + +@pytest.mark.parametrize("argv", _EMPTY_FLAGS, ids=lambda argv: " ".join(argv[:-1])) +def test_an_empty_flag_is_an_error(tmp_path, monkeypatch, argv): + # An empty value must not mean the current directory: a build would land there, and + # `local clear` would delete its releases. + monkeypatch.chdir(tmp_path) + result = runner.invoke(app, argv) + assert isinstance(result.exception, TimeFValidationError) + assert f"must not be empty (also as {argv[-2]})" in str(result.exception) + + def test_search_rejects_unknown_filter_value(registry_root): - assert runner.invoke(app, ["search", "--registry", str(registry_root), "--domain", "bogus"]).exit_code == 2 + assert runner.invoke(app, ["search", "--remote-registry", str(registry_root), "--domain", "bogus"]).exit_code == 2 # The error message is a Rich-rendered panel, so assert it on the raising helper directly. with pytest.raises(typer.BadParameter, match="invalid --domain"): _enum_list(["bogus"], Domain, "--domain", [d.value for d in Domain]) diff --git a/packages/timenet/tests/test_client.py b/packages/timenet/tests/test_client.py index dacc64a74..282360a66 100644 --- a/packages/timenet/tests/test_client.py +++ b/packages/timenet/tests/test_client.py @@ -445,8 +445,8 @@ def test_download_counts_a_registry_over_the_directory_of_the_store_as_the_store if case == "damaged": (directory / "control.duckdb").write_bytes(b"cut short") before = {path: path.read_bytes() for path in directory.rglob("*") if path.is_file()} - # As for `TIMENET_LOCAL_REGISTRY=./reg timenet download timenet/hello-world --registry ./reg`: the - # registry after the store reads the directory of the store, so it cannot replace its copy. + # As for `timenet download timenet/hello-world --local-registry ./reg --remote-registry ./reg`: + # the registry after the store reads the directory of the store, so it cannot replace its copy. monkeypatch.setenv("TIMENET_LOCAL_REGISTRY", str(root)) client = TimeNet(sources=default_sources(remote_registry=root)) diff --git a/packages/timenet/tests/test_config.py b/packages/timenet/tests/test_config.py index d3b5fe5b4..f5cdabb47 100644 --- a/packages/timenet/tests/test_config.py +++ b/packages/timenet/tests/test_config.py @@ -73,12 +73,22 @@ def test_an_empty_or_blank_setting_raises_instead_of_naming_a_directory(monkeypa settings() -def test_an_empty_argument_raises_with_the_text_of_its_variable(): - with pytest.raises( - TimeFValidationError, - match=r"^TIMENET_REMOTE_REGISTRY must not be empty; give it a value, or leave it unset for timenet://\.$", - ): - settings(remote_registry="") +@pytest.mark.parametrize( + ("name", "message"), + [ + ( + "remote_registry", + "TIMENET_REMOTE_REGISTRY must not be empty (also as --remote-registry); give it a value, " + "or leave it unset for timenet://.", + ), + ("cache", "TIMENET_CACHE must not be empty; give it a value, or leave it unset for ~/.cache/timenet."), + ], +) +def test_an_empty_argument_raises_with_the_text_of_its_variable_and_its_flag(name, message): + # TIMENET_CACHE has no flag, so its message names only the variable. + with pytest.raises(TimeFValidationError) as error: + settings(**{name: ""}) + assert str(error.value) == message def test_an_argument_replaces_an_invalid_variable(monkeypatch, tmp_path):