diff --git a/docs/branch/py4lexis4-cmd.md b/docs/branch/py4lexis4-cmd.md index fee98a5..388db53 100644 --- a/docs/branch/py4lexis4-cmd.md +++ b/docs/branch/py4lexis4-cmd.md @@ -99,6 +99,207 @@ def less(self, local_specifier: str, remote_specifier: str, ...): --- +### Parameter Parsing Deep Dive + +#### Current Flow: CLI → Command + +``` +CLI Input: owi remote ls all files="['**/*']" format=wide + + ↓ Click captures + +@click.argument('subcmd') # "ls" +@click.argument('specifier') # "all" +@click.argument('args', nargs=-1) # ("files=['**/*']", "format=wide") + + ↓ extract_args() + +args = [] +kwargs = {"files": "['**/*']", "format": "wide"} + + ↓ RemoteCommands.do() + + ↓ _cast_args(func, args, kwargs) + +Pydantic TypeAdapter converts types based on function signature +``` + +#### `extract_args()` in cli.py (Lines 212-238) + +```python +def extract_args(args): + """ + Separates positional and keyword arguments from CLI args. + - Keyword: contains '=' and NOT wrapped in '{...}' + - Positional: everything else + - Escape: wrap in {} to pass literal '=' (e.g., "{a=b}") + """ + def is_kwargs(x: str): + return "=" in x and not (x.startswith('{') and x.endswith('}')) + + kwargs = {k[0]: k[1] for k in [f.split("=", 1) for f in args if is_kwargs(f)]} + args = [arg for arg in args if not is_kwargs(arg)] + return args, kwargs +``` + +**Issues:** +1. Non-standard: `key=value` instead of `--key value` +2. String escaping awkward: `files="['**/*']"` (needs quotes) +3. `{...}` escape mechanism undocumented +4. `-` to `_` conversion (e.g., `pq-batch-size` → `pq_batch_size`) + +#### `_cast_args()` in base.py (Lines 240-307) + +```python +def _cast_args(self, func, args, kwargs): + """ + Uses pydantic TypeAdapter to convert args/kwargs to match + function signature types. + """ + sig = inspect.signature(func) + for param in sig.parameters.values(): + if param.annotation != inspect.Parameter.empty: + type_adapter = TypeAdapter(expected_type) + converted = type_adapter.validate_python(value) +``` + +**Pros:** +- Automatic type conversion (str → int, str → bool) +- Validation via pydantic + +**Cons:** +- Complex error handling +- Hidden behavior (user doesn't see types) + +#### Example: Query Less Command + +```bash +# Current syntax +owi query less --local . --remote main:latest select=url,title limit=10 + +# Parsed as: +subcmd = "less" +local = "." +remote = "main:latest" +args = [] +kwargs = {"select": "url,title", "limit": "10"} # ← limit is string! + +# _cast_args converts: +kwargs = {"select": "url,title", "limit": 10} # ← now int +``` + +--- + +### Specifier Format (KEEP) + +The specifier is a powerful mini-DSL for dataset selection. **✅ Worth keeping and documenting properly.** + +#### Syntax + +``` +:#/=;=/... +``` + +#### Components + +| Part | Format | Required | Default | Example | +|------|--------|----------|---------|---------| +| `datacenter` | `all`, `lrz`, `it4i`, `csc` | Yes | `all` | `lrz` | +| `day` | `YYYY-MM-DD`, `latest`, `*` | No | - | `2024-03-24`, `latest` | +| `duration` | `#N` (days) | No | - | `#7` (last 7 days) | +| `filters` | `/key=value;key=value` | No | - | `/access=public` | + +#### Parsing (manager.py:772-819) + +```python +def parse_specifier(self, specifier: str) -> dict: + """ + Regex: ^(?P[\w-]+)(?::(?P[\d-]+|latest)(?:#(?P\d+))?)?$ + """ + result = { + 'data_center': None, # "lrz", "it4i", "csc", None for "all" + 'day': None, # datetime or "latest" + 'duration': None, # int (days) + 'query': {} # {"key": "value", ...} + } +``` + +#### Examples + +| Specifier | Meaning | +|-----------|---------| +| `all` | All datasets from all data centers | +| `lrz:latest` | Latest datasets from LRZ | +| `it4i:2024-03-24` | Datasets from IT4I on specific date | +| `csc:2024-01#7` | CSC datasets, Jan 2024, 7 days | +| `all/access=public` | All public datasets | +| `lrz:latest/collectionName=curlie` | Latest Curlie from LRZ | +| `all/id=f6ea5756-...` | Specific dataset by ID | + +#### Design Recommendations for Typer + +Keep specifier as a **first positional argument** for brevity: + +```bash +# Current (good - keep) +owi remote ls lrz:latest +owi remote pull all:2024-01#7/access=public + +# Alternative with Typer (proposed) +owi remote ls lrz:latest # Argument +owi remote ls --spec lrz:latest # Option (if needed for clarity) +``` + +--- + +### Typer Migration: Parameter Strategy + +#### Proposed Conventions + +| Element | Typer | Example | +|---------|-------|---------| +| Required positional | `typer.Argument()` | `specifier: str` | +| Optional with default | `typer.Option()` | `--limit 10` | +| Boolean flags | `typer.Option(is_flag=True)` | `--verbose` | +| Multiple values | `List[str]` | `--file f1 --file f2` | + +#### Example: `owi remote ls` + +**Current:** +```bash +owi remote ls all format=wide files="['**/*']" no_summary=true +``` + +**Proposed Typer:** +```bash +owi remote ls all --format wide --files "**/*" --no-summary +owi remote ls lrz:latest -f wide --files "**/*" +``` + +**Implementation:** +```python +@remote_app.command() +def ls( + specifier: str = typer.Argument("all", help="Dataset specifier"), + format: str = typer.Option("short", "--format", "-f", help="Output format"), + files: Optional[str] = typer.Option(None, help="File glob pattern"), + no_summary: bool = typer.Option(False, "--no-summary", help="Skip summary"), + details: bool = typer.Option(False, "--details", "-l", help="Show details") +): + """List datasets matching SPECIFIER.""" +``` + +#### Balancing Brevity and Extensibility + +| Goal | Approach | +|------|----------| +| Short commands | Specifier as positional `Argument` | +| Common options | Short aliases (`-f`, `-l`, `-v`) | +| Extensibility | All other options as `--key value` | +| Backward compat | Support `key=value` via custom parser (optional) | + +--- + ### Documentation | Location | Content |