Skip to content

Add filtering, paging, and richer output to all list commands - #9

Open
151N3 wants to merge 5 commits into
onefloid:mainfrom
151N3:main
Open

Add filtering, paging, and richer output to all list commands#9
151N3 wants to merge 5 commits into
onefloid:mainfrom
151N3:main

Conversation

@151N3

@151N3 151N3 commented Aug 14, 2026

Copy link
Copy Markdown

Improve List Filtering, Output Formats, and CLI Validation

This pull request was prepared with the assistance of GitHub Copilot.

Summary

This pull request improves the list commands for cubes, dimensions, processes, subsets, and views.

It adds predictable filtering, multiple output formats, paging support, stronger validation, clearer documentation, and additional regression coverage.

Changes

Filtering

  • --filter now performs a literal, case-insensitive substring match.
  • Added a separate --regex option for regular-expression filtering.
  • Invalid regular expressions return a clear CLI validation error.

Examples:

tm1cli cube list --filter "Sales(EMEA)"
tm1cli cube list --regex "^Sales_.*"

Output and Paging

All list commands support:

  • --output yaml
  • --output json
  • --output plain
  • --limit
  • --offset
  • Global --output-raw for scripting

The filtering, paging, and rendering pipeline is shared through tm1cli/utils/list_utils.py.

View and Subset Commands

  • Added public, private, or both visibility selection through --type.
  • Added structured output containing:
    • Parent cube or dimension
    • Hierarchy where applicable
    • Object name
    • Visibility type
  • Added explicit control-object options:
    • view list --skip-control-cubes
    • subset list --skip-control-dims
  • --hierarchy now requires --dimension.
  • Updated command help text to document the request cost when scanning all cubes or dimensions.

Validation and Bug Fixes

  • Fixed view list handling of TM1py's (private, public) return value.
  • Fixed the threads --beautify crash when no threads are returned.
  • Rejected negative values for --limit.
  • Corrected optional database parameter typing to Optional[str].
  • Removed duplicate list-command logic that triggered Pylint duplicate-code warnings.
  • Preserved plain output behavior for scripts using --output-raw.

Documentation and Release Metadata

  • Updated README command examples and list options.
  • Added breaking changes and fixes to the changelog.
  • Bumped the package version from 0.2.0 to 0.3.0.
  • Documented the deliberate use of typer.echo for YAML and JSON output.

N+1 Request Behavior

TM1py 2.x does not expose a bulk API for listing views across all cubes or subsets across all dimensions.

The implementation therefore:

  • Requests only the selected subset visibility.
  • Uses the existing TM1py view API without relying on private REST internals.
  • Documents the request cost when scanning all cubes or dimensions.

A complete reduction of view requests requires either:

  • A supported bulk-listing API in TM1py.
  • A confirmed server-side bulk endpoint.
  • A future TM1py enhancement exposing bulk view and subset metadata.

Breaking Changes

  • view list CUBE_NAME is now:

    tm1cli view list --cube CUBE_NAME
  • subset list DIMENSION_NAME is now:

    tm1cli subset list --dimension DIMENSION_NAME
  • List commands now default to YAML list output instead of one raw name per line.

  • dimension list --skip-control-cubes is replaced by --skip-control-dims.

For scripting, use:

tm1cli --output-raw cube list

or:

tm1cli cube list --output plain

Testing

The following validation was completed:

  • 51 tests passed.
  • 0 test failures.
  • Pylint score: 10.00/10.
  • No Pylint findings.
  • git diff --check passes.
  • No merge conflict markers remain.
  • Only existing TM1py deprecation warnings remain.

Review Notes

The implementation avoids direct access to TM1py's private REST internals because those APIs are version-dependent.

The remaining request expansion for view list across all cubes is documented and should be addressed in a future TM1py-supported bulk metadata API.

onefloid

This comment was marked as duplicate.

@onefloid onefloid left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 list really was broken on main. Per the TM1py source, ViewService.get_all_names() returns a Tuple[List[str], List[str]] (private, public); the old code iterated over that tuple. Reproduced on main, the output was:
    ['Priv1']
    ['View1', 'View2']
    
    This was invisible only because MockedViewService returned a flat list — so the mock fix in conftest.py is 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-dims on dimension list: a real copy-paste fix.
  • The threads --beautify IndexError guard: reproducible crash on an empty thread list, cleanly fixed.
  • Switching from rich.print to typer.echo is 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 in CLAUDE.md and 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

⚠️ The workflow is triggered 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 syntax
  • view list CUBE_NAME (positional) → view list --cube CUBE_NAME — README line 62 likewise
  • Output format of all five list commands: 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 / --output on cube, dimension, subset, view
  • --type private and --type both
  • all the -s flags (the mocks ignore skip_control_* anyway, so a test wouldn't actually assert anything)
  • the threads empty-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 Annotated import from typing_extensions removed" doesn't apply — main already imports exclusively from typing.
  • The subset records also carry a hierarchy field, which the description doesn't mention.

Nits

  • output: Annotated[OutputFormat, ...] = "yaml" — better to default to the enum member OutputFormat.yaml than to the raw string.
  • --cube has the short flag -c, --dimension has none. Deliberate, because of the clash with -d (= database)? If so, maybe -D.
  • subset.py and view.py share ~10 identical lines of filter/render logic (this is what triggers R0801) → consider lifting it into list_utils.py, e.g. as filter_records(records, key, pattern).
  • tests/test_tm1cli.py:67 is 133 characters long (not linted, but out of step with the surrounding style).
  • The PR head is the fork's main branch — a feature branch would be more practical for follow-ups.

Generated by Claude Code

@151N3

151N3 commented Sep 10, 2026

Copy link
Copy Markdown
Author

@onefloid Thanks for the detailed review. I addressed the merge blockers and functional issues:

  • Pylint now passes with 10.00/10.
  • Added validation for --hierarchy without --dimension.
  • Added positive validation for --limit.
  • Changed --filter to literal case-insensitive substring matching.
  • Added explicit --regex support.
  • Added --output plain and support for global --output-raw.
  • Added regression tests for the new validation and output behavior.
  • Updated the README, CHANGELOG, and package version to 0.3.0.

Regarding the N+1 request behavior: I checked the TM1py 2.x implementation. It does not expose a supported bulk API for listing views across cubes or subsets across dimensions. The implementation therefore avoids private TM1py internals and documents the request cost in the command help. A complete optimization requires a supported bulk API in TM1py.

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 list really was broken on main. Per the TM1py source, ViewService.get_all_names() returns a Tuple[List[str], List[str]] (private, public); the old code iterated over that tuple. Reproduced on main, the output was:

    ['Priv1']
    ['View1', 'View2']
    

    This was invisible only because MockedViewService returned a flat list — so the mock fix in conftest.py is 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-dims on dimension list: a real copy-paste fix.

  • The threads --beautify IndexError guard: reproducible crash on an empty thread list, cleanly fixed.

  • Switching from rich.print to typer.echo is 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 in CLAUDE.md and 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
⚠️ The workflow is triggered 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 syntax
  • view list CUBE_NAME (positional) → view list --cube CUBE_NAME — README line 62 likewise
  • Output format of all five list commands: 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 / --output on cube, dimension, subset, view
  • --type private and --type both
  • all the -s flags (the mocks ignore skip_control_* anyway, so a test wouldn't actually assert anything)
  • the threads empty-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 Annotated import from typing_extensions removed" doesn't apply — main already imports exclusively from typing.
  • The subset records also carry a hierarchy field, which the description doesn't mention.

Nits

  • output: Annotated[OutputFormat, ...] = "yaml" — better to default to the enum member OutputFormat.yaml than to the raw string.
  • --cube has the short flag -c, --dimension has none. Deliberate, because of the clash with -d (= database)? If so, maybe -D.
  • subset.py and view.py share ~10 identical lines of filter/render logic (this is what triggers R0801) → consider lifting it into list_utils.py, e.g. as filter_records(records, key, pattern).
  • tests/test_tm1cli.py:67 is 133 characters long (not linted, but out of step with the surrounding style).
  • The PR head is the fork's main branch — a feature branch would be more practical for follow-ups.

Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants