Add filtering, paging, and richer output to all list commands - #9
Conversation
Co-authored-by: Copilot <copilot@github.com>
onefloid
left a comment
There was a problem hiding this comment.
Note
🤖 This review was written by an AI (Claude Code) — on behalf of @onefloid, but not read line by line by a human. The findings were verified locally (branch checked out, pylint and pytest run, signatures checked against the TM1py 2.x source), so the commands and outputs quoted below are reproducible. Even so: please check them critically and push back where you disagree. The design points (5–8) are suggestions, not directives.
ℹ️ This replaces the earlier German-language review on this PR — same content, corrected formatting. Please use this one.
Thanks for the PR — the direction is right, and there's a genuine bug fix in here. That said, I don't think it's mergeable as-is: one hard CI blocker, one functional bug, and several undocumented breaking changes.
What's good (verified)
view listreally was broken onmain. Per the TM1py source,ViewService.get_all_names()returns aTuple[List[str], List[str]](private, public); the old code iterated over that tuple. Reproduced onmain, the output was:This was invisible only because['Priv1'] ['View1', 'View2']MockedViewServicereturned a flat list — so the mock fix inconftest.pyis correct and uncovers a real defect.- All the changed TM1py calls match the actual signatures (
skip_control_cubes/skip_control_dims/skip_control_processes,subsets.get_all_names(dimension_name, hierarchy_name, private)). --skip-control-cubes→--skip-control-dimsondimension list: a real copy-paste fix.- The
threads --beautifyIndexError guard: reproducible crash on an empty thread list, cleanly fixed. - Switching from
rich.printtotyper.echois the right call here — Rich would interpret TM1 names containing square brackets as markup and mangle the YAML/JSON. This is a deliberate departure from the convention inCLAUDE.mdand should be recorded there as an exception.
Blockers
1. The pylint CI job fails
main scores 10.00/10 (exit 0); this branch scores 8.90/10, exit 28 → .github/workflows/pylint.yml would go red. 29 findings:
| Type | Count | Where |
|---|---|---|
C0301 line-too-long (124–127 / max 120) |
10 | all five commands/*.py, 2 lines each |
W0622 redefined-builtin filter / type |
6 | cube, dimension, process, subset, view |
W0611 unused-import print from rich |
4 | cube, dimension, subset, view |
C0103 invalid-name (enum members) |
5 | list_utils.py |
R0914 too-many-locals (18/15) |
2 | subset, view |
R0801 duplicate-code |
1 | subset ↔ view |
on: [push], which does not fire for fork PRs in the upstream repo (this PR currently has 0 check runs). So the failure would only become visible after the merge to main.
Suggested fix: rename the parameters internally to name_filter / output_format (the CLI surface stays identical via typer.Option("--filter", "-f", ...) and typer.Option("--type", "-t", ...)), wrap the long lines, drop the now-unused print import, and add a # pylint: disable=invalid-name in list_utils.py.
2. Bug: --hierarchy without --dimension
In tm1cli/commands/subset.py, hier = hierarchy or dim is evaluated while iterating over all dimensions. So tm1cli subset list --hierarchy Leaves queries the hierarchy Leaves on every dimension — against a real server that raises exceptions/404s for nearly all of them. The mock hides this because it ignores hierarchy_name.
--hierarchy should require --dimension and otherwise bail out via print_error_and_exit.
3. Breaking changes with no documentation
git diff --name-only main..HEAD shows that README, CHANGELOG and pyproject.toml are untouched. Affected:
subset list DIMENSION_NAME(positional) →subset list --dimension DIMENSION_NAME— README line 65 still shows the old syntaxview list CUBE_NAME(positional) →view list --cube CUBE_NAME— README line 62 likewise- Output format of all five
listcommands:Name→- Name dimension list --skip-control-cubes→--skip-control-dims
For a package published on PyPI (currently 0.2.0) this needs a ### Breaking changes section in the CHANGELOG and a version bump — see "Releasing" in CLAUDE.md.
Other findings
4. --limit accepts negative values
--limit -1 silently returns every entry except the last (items[:-1]) instead of erroring:
$ tm1cli cube list --limit -1
- Cube1 # Cube2 disappears with no message
--offset correctly sets min=0; --limit is missing min=1.
5. Silent regex fallback in apply_filter
On re.error the function falls back to substring matching without a word. cube list --filter "cube[" returns [] with no hint that the pattern was invalid. On top of that: TM1 object names frequently contain (, ), . — so --filter "Sales(EMEA)" is interpreted as a regex with a group and does not match the literal name.
Suggestion: define --filter as substring/glob and offer a separate --regex; at the very least, abort via print_error_and_exit on an invalid regex rather than silently switching semantics.
6. Inconsistent handling of control objects
cube list and dimension list show control objects by default (-s is opt-in), but view list and subset list hard-wire skip_control_cubes=True / skip_control_dims=True with no flag to include them. Pick one: opt-in everywhere, or opt-out everywhere.
7. The global --output-raw is ignored by the list commands
There are now two independent output-control mechanisms (--output-raw on the callback, --output per command), and no way back to plain-text output. tm1cli cube list | while read name; do ...; done breaks because of the - prefix. Suggestion: either add --output plain or honour ctx.obj["raw"] in the list commands.
8. N+1 requests on the new default paths
view list without --cube issues 1 + 2 × (number of cubes) REST calls — TM1py fetches private and public views in separate requests, even for --type public. subset list without --dimension does the same per dimension. On large models this is noticeably slow, with no progress indication. Worth mentioning in the help text at minimum.
9. Test gaps
The 28 mocked tests pass for me — but the new flags are covered only for process. Not covered:
--filter/--limit/--offset/--outputon cube, dimension, subset, view--type privateand--type both- all the
-sflags (the mocks ignoreskip_control_*anyway, so a test wouldn't actually assert anything) - the
threadsempty-list fix
Also, MockedSubsetService returns the same names for private=True as for private=False, so --type both produces duplicates in the tests without that standing out.
10. Two claims in the PR description don't hold
- The claimed fix "duplicate
Annotatedimport fromtyping_extensionsremoved" doesn't apply —mainalready imports exclusively fromtyping. - The subset records also carry a
hierarchyfield, which the description doesn't mention.
Nits
output: Annotated[OutputFormat, ...] = "yaml"— better to default to the enum memberOutputFormat.yamlthan to the raw string.--cubehas the short flag-c,--dimensionhas none. Deliberate, because of the clash with-d(= database)? If so, maybe-D.subset.pyandview.pyshare ~10 identical lines of filter/render logic (this is what triggersR0801) → consider lifting it intolist_utils.py, e.g. asfilter_records(records, key, pattern).tests/test_tm1cli.py:67is 133 characters long (not linted, but out of step with the surrounding style).- The PR head is the fork's
mainbranch — a feature branch would be more practical for follow-ups.
Generated by Claude Code
Summary
Upgrades all five
listcommands (cube, dimension, process, subset, view) with a consistent setof new options, fixes a code bug, hardens the test suite, and patches a crash in the
threadscommand.New features
Shared utility (
tm1cli/utils/list_utils.py)New module with
OutputFormat(yaml/json),VisibilityType(public/private/both),apply_filter,apply_paging, andrender_output— reused by all list commands.Common new options on
cube list,dimension list,process list--filter/-f--output/-oyaml(default) orjson--limit--offsetOutput format changed from one name per line to a YAML/JSON list (
- Name).subset listandview list— richer structured outputdimension_name/cube_namearguments replaced by optional--dimension/--cubeflags; omitting them iterates over all non-control dimensions/cubes.
--type/-tflag:public(default),private, orboth.dimension/cube,name, andtype.process list— skip control TIsNew
--skip-control-tis/-sflag to exclude processes whose names start with}.dimension listFlag renamed from
--skip-control-cubes→--skip-control-dims(was a copy-paste mistake).Bug fixes
process.py— duplicateAnnotatedimport removedfrom typing_extensions import Annotatedwas left in place after the newfrom typing import Annotated, Optionalwas added, silently shadowing it.The stale
typing_extensionsimport was removed.main.py—threads --beautifycrash on empty listthreads[0].keys()raisedIndexErrorwhen no threads were active for the current session.Added an empty-list guard; prints
"No threads."in that case.Test improvements
tests/conftest.pyMockedCubeService.get_all_namessignature fixed to match the real API (skip_control_cubeskwarg).MockedViewService.get_all_namesnow returns a(private, public)tuple matching TM1py's actual return type.MockedSubsetService.get_all_namesextended withhierarchy_nameandprivateparameters.MockedProcessServiceadded withget_all_names,exists,get, andupdate_or_create.New
tests/test_cmd_process.py8 mocked unit tests covering
list/lsaliases, JSON output (--output json),--filter,--limit,--offset, andexiststrue/false.tests/test_tm1cli.pytest_process_clone_not_existsconverted to a mocked test: patches bothTM1Serviceandresolve_databaseso the process-not-found path is reached before any database-lookup error.