refactor: migrate criteria-limit ad-target get to make_get_command (part of #587)

[axisrow]

What

Migrate dynamicads get, smartadtargets get and dynamicfeedadtargets get — three byte-for-byte identical hand-rolled command bodies — onto the shared make_get_command factory (direct_cli/commands/_get.py), following the dedup epic #584 / factory introduced in #582.

Net −131 lines (91 added, 222 removed).

Factory extension (two generic optional params)

• criteria_limits — {SelectionCriteria key: max count} dict, forwarded to the existing enforce_criteria_array_limits util. Command name for the error is derived as f"{group.name} get" (== the previous hardcoded "dynamicads get" etc.).
• require_criteria_message — i18n key; an empty SelectionCriteria raises UsageError with this message (the "provide at least one filter" guard).
• New shared ids_adgroup_campaign_states_criteria builder for the Ids/AdGroupIds/CampaignIds/States selection shape (composes on the existing _default_ids_criteria).

Both new params default to None, so the 5 existing factory callers (advideos/businesses/turbopages/vcards/negativekeywordsharedsets) are untouched.

Byte-identity (hard invariant of #587)

Verified each command against pre-migration baselines — all 15 snapshots identical:

• --help (option order, i18n keys)
• --dry-run payload (typed-flags + states)
• over-limit error (CampaignIds cap)
• empty-criteria error
• per-module create_client patchability (via resolve_module_create_client)

Tests

• Full offline suite green (2552 passed); test_dry_run, test_api_coverage, test_comprehensive, test_read_cassettes included.
• ruff check clean (dead imports from the removed get bodies dropped).

Scope

Part of #587 — Group 3 (criteria-limits commands). The remaining work follows in a separate PR:

• Group 2 (nested *FieldNames): adextensions, adimages, leads, retargeting, sitelinks, feeds, clients, agencyclients.
• No---ids / nested-field hybrids: bids, keywordbids, bidmodifiers (need factory support for omitting the mandatory --ids and/or nested_field_options).
• reports get is intentionally excluded (custom TSV-stream endpoint, not a SOAP get).

🤖 Generated with Claude Code

[axisrow]

@claude @codex review. Focus on critical issues: bugs, security vulnerabilities, logical errors, data loss risks, performance problems. Do NOT nitpick style, naming conventions, minor formatting, or subjective preferences — only flag issues that could break functionality or cause real harm in production.

[claude]

Claude finished @axisrow's task in 2m 9s —— View job

---

Review: PR #597 — criteria-limit get migration

Verdict: LGTM — no critical issues found. This is a faithful, mechanical dedup of three byte-for-byte identical get bodies onto the shared factory. I traced every behavioral path against the pre-migration code.

Critical-path verification (all preserved)

Concern	Original	After migration	OK
Order: limits enforced before empty-criteria guard	enforce… then if not criteria	_get.py:119-124 same order	✅
Error command name	hardcoded "dynamicads get"	f"{svc} get", svc = group.name	✅
Client service call	client.dynamicads()	getattr(client, svc)(), svc == group.name	✅
Criteria shape (Ids/AdGroupIds/CampaignIds/States, key order)	inline	ids_adgroup_campaign_states_criteria _get.py:44-61	✅
Empty --states → {"States": []} (truthy → guard not tripped)	parse_csv_upper(states) or []	identical	✅
--dry-run / fetch_all / extract paths	inline	_get.py:130-149	✅
create_client patchability	client_from_ctx	resolve_module_create_client (lifecycle contract)	✅

i18n: checked the one risk that could have bitten

Moving t("Provide at least one typed filter") out of the module and into _get.py (now t(require_criteria_message)) is safe — i18n._load_translations merges all translations/*.json into a single global _RU table keyed by source string (i18n.py:41-63). Lookup is not caller-module-scoped, so resolving the key from _get.py yields the same Russian string. The literal is still present in source and in translations/dynamicfeedadtargets.json, so it remains catalog-covered. No localization regression.

Minor observations (non-blocking, not introduced by this PR)

• command_name=f"{svc} get" relies on group.name == client service attr name. True for all three modules here and asserted indirectly by the byte-identity snapshots, but it's now a structural assumption baked into the factory — worth a one-line note if a future resource ever has a group name that diverges from its API service name.
• Passing --states with only empty/whitespace tokens still emits {"States": []} and skips the "provide a filter" guard. This is pre-existing behavior, faithfully preserved — flagging only so it isn't mistaken for new.

Note

I couldn't execute pytest in this environment (commands required interactive approval), so I relied on static line-by-line comparison rather than a local run. The PR reports the offline suite green (2552 passed) including test_dry_run, test_api_coverage, test_comprehensive, and test_read_cassettes, and ruff clean — consistent with my read (no dead imports; t still used by the add/update bodies in each module).

Clean refactor, −131 lines, behavior preserved. 👍
• feat/587-group3-criteria-limits

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 1c03b05f5e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".