Skip to content

feat: allow repeated --filter flags for exports - #443

Open
jacalata wants to merge 3 commits into
developmentfrom
jac/filter-repeatable
Open

jacalata wants to merge 3 commits into
developmentfrom
jac/filter-repeatable

Conversation

@jacalata

@jacalata jacalata commented Jul 29, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

--filter previously accepted one value and required multi-pair filters
to be joined with &. That parsing scheme prevents literal & inside a
value (e.g. Product Name=AT&T gets split). Making --filter
repeatable removes the need for callers to encode delimiters.

Related to #442, which handles the same problem when the filter comes in
via URL syntax rather than the flag.

Behavior change

For users: --filter is now repeatable. Each flag carries exactly
one COLUMN=VALUE pair.

--filter "Region=West&Product=Widget"                # still works (back-compat)
--filter "Region=West" --filter "Product=Widget"     # preferred
--filter "Product=AT&T"                              # new: repeated form allows '&' in value

Back-compat: when only one --filter is present, the value is still
split on &. This means a single flag carrying one filter with & in
its value (e.g. --filter "Product=AT&T" alone) is still ambiguous.
Workaround: pass the filter in the URL query string instead of via
--filter. #442 landed the URL-path handling for literal &, so
tabcmd get "views/View/Sheet.csv?Product%20Name=AT&T%20841000%20Phone"
works correctly (space must be encoded as %20; & may be literal).

Test plan

  • pytest tests/commands/test_datasources_and_workbooks_command.py tests/parsers/test_parser_export.py — 34 passed
  • Manual e2e: repeated --filter with & in the value returns the
    expected row
  • Manual e2e: single-flag &-joined form still parses and filters
    correctly

🤖 Generated with Claude Code

Previously `--filter` accepted only one value, and multi-pair filtering
required joining pairs with '&' (e.g. `--filter "A=1&B=2"`). That
parsing scheme conflicts with values that legitimately contain '&'
(e.g. `Product Name=AT&T ...`).

Make `--filter` repeatable via argparse action="append". Each flag now
carries exactly one COLUMN=VALUE pair — no '&' splitting — so literal
'&' in a value passes through untouched.

Back-compat: when only one --filter flag is present, the value is still
split on '&' so existing scripts keep working. Users who need '&' in a
value should switch to the repeated-flag form.

Adds parser tests for the new syntax and unit tests for
apply_filters_from_args covering both forms.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jacalata
jacalata changed the base branch from main to development July 30, 2026 21:57
@jacalata
jacalata requested a lite review from Copilot August 6, 2026 21:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds support for repeating --filter on the export command while preserving backward-compatible parsing of legacy &-joined filter pairs.

Changes:

  • Update export CLI arg parsing to accept multiple --filter flags (action="append").
  • Implement back-compat filter splitting when a single --filter contains &.
  • Add/extend tests for parser behavior and filter application; update help text to document new usage.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
tests/parsers/test_parser_export.py Adds tests validating --filter repeatability and default behavior.
tests/commands/test_datasources_and_workbooks_command.py Adds unit tests for applying parsed filters into TSC request options, including back-compat.
tabcmd/locales/en/tabcmd_messages_en.properties Expands the --filter help text to document the new repeatable flag semantics and back-compat.
tabcmd/commands/datasources_and_workbooks/export_command.py Changes --filter arg to append and updates filter parsing logic accordingly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tabcmd/commands/datasources_and_workbooks/export_command.py
All call sites pass logger explicitly, so the default was unreachable.
Tightens the type from Optional[Logger] to Logger, drops any fallback
branch inside.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 10, 2026 19:49
@github-actions

github-actions Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

Coverage

Coverage Report
FileStmtsMissCoverMissing
tabcmd
   __main__.py121212 0%
   _version.py111111 0%
   tabcmd.py151515 0%
   version.py1066 40%
tabcmd/commands
   commands.py101010 0%
   constants.py771818 77%
   server.py1351818 87%
tabcmd/commands/auth
   session.py3945050 87%
tabcmd/commands/datasources_and_workbooks
   datasources_and_workbooks_command.py1602020 88%
   datasources_workbooks_views_url_parser.py14255 96%
   delete_command.py702323 67%
   export_command.py1262121 83%
   get_url_command.py1274747 63%
   publish_command.py1341616 88%
   runschedule_command.py2177 67%
tabcmd/commands/extracts
   create_extracts_command.py4288 81%
   decrypt_extracts_command.py2722 93%
   delete_extracts_command.py3766 84%
   encrypt_extracts_command.py2722 93%
   extracts.py2022 90%
   reencrypt_extracts_command.py2722 93%
   refresh_extracts_command.py481010 79%
tabcmd/commands/group
   create_group_command.py2955 83%
   delete_group_command.py2722 93%
tabcmd/commands/project
   create_project_command.py4688 83%
   delete_project_command.py3544 89%
   publish_samples_command.py3044 87%
tabcmd/commands/site
   create_site_command.py3455 85%
   delete_site_command.py2722 93%
   edit_site_command.py661818 73%
   list_command.py771212 84%
   list_sites_command.py2922 93%
tabcmd/commands/user
   add_users_command.py2955 83%
   create_site_users.py581111 81%
   create_users_command.py5999 85%
   delete_site_users_command.py4355 88%
   user_data.py2223131 86%
tabcmd/execution
   _version.py222 0%
   global_options.py14788 95%
   localize.py671111 84%
   logger_config.py6066 90%
   tabcmd_controller.py4277 83%
TOTAL295546884% 

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The updated filter-path still risks truncating values containing = and can error on ambiguous single-flag & splits unless parsing is made more robust (and covered by a regression test).

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

tabcmd/commands/datasources_and_workbooks/export_command.py:150

  • apply_filters_from_args currently claims repeated flags allow literal '&' or '=' inside values, but the implementation delegates to DatasourcesAndWorkbooks.apply_filter_value, which splits on every '=' and will truncate values like Notes=x=y. Also, the back-compat single-flag split on '&' can yield fragments without '=', which can raise IndexError downstream. Consider parsing COLUMN=VALUE here with split('=', 1) and only treating '&' as a delimiter when every segment contains '=' (otherwise treat '&' as literal and warn) to avoid crashes and preserve values.
    tests/commands/test_datasources_and_workbooks_command.py:217
  • Current tests cover '&' in values, but they don't assert the stated behavior that '=' inside a filter value is preserved (e.g. Notes=x=y). Adding a regression test would catch truncation bugs in filter parsing.
  • Files reviewed: 4/4 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

- apply_filter_value now splits on the first '=' only, so values
  containing '=' (e.g. Notes=x=y) are no longer truncated.
- apply_filters_from_args only treats a literal '&' in a single
  --filter value as a back-compat pair delimiter when every resulting
  segment actually looks like COLUMN=VALUE; otherwise the '&' is
  treated as literal (e.g. Company=AT&T), avoiding the previous
  IndexError risk on ambiguous splits.
- Add regression tests for both cases.

Co-Authored-By: Claude Code <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings October 20, 2026 23:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Invalid filter handling and documented URL ampersand parsing remain unresolved.

1 open issue
2 previously missed issues

In code that hasn't changed since last review

Medium severity Preserve ampersands in URL filter values

tabcmd/​commands/​datasources_and_workbooks/​datasources_and_workbooks_command.py:111

URL filters are still split on every & in apply_values_from_url_params before this method is called. Consequently ?Product%20Name=AT&T%20841000%20Phone is applied as Product Name=AT and this guard merely skips the T 841000 Phone fragment, so the literal ampersand is not preserved and the result is incorrect. Rejoin URL fragments that are part of a value (or otherwise handle them in the URL parser) before calling apply_filter_value, and add a regression test for the documented URL workaround.

Low severity Regenerate compiled catalogs with updated filter help

tabcmd/​locales/​en/​tabcmd_messages_en.properties:158

The updated help text is only in the .properties source, but the application loads tabcmd/locales/*/LC_MESSAGES/tabcmd.mo at runtime and the checked-in English catalog still contains the old translation. As a result, the packaged/normal CLI help will not describe repeatable filters until the localization pipeline regenerates and includes the compiled catalogs.

Review effort: Lite

Comment on lines +109 to +111
if len(data_filter) != 2:
logger.warning("Unable to read filter param '{}', expected COLUMN=VALUE. Skipping".format(value))
return

This branch has not been deployed

No deployments
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