refactor: migrate nested-*FieldNames get to make_get_command (part of #587)

[axisrow]

What

Migrate adimages get, retargeting get, sitelinks get, feeds get and clients get onto the shared make_get_command factory (dedup epic #584). Follows PR #597 (Group 3 / criteria-limits).

Net −175 lines (205 added, 380 removed).

Factory extension (three generic optional params)

• nested_field_options — tuple of (flag, WSDL key, help) for nested *FieldNames projections (e.g. ("--sitelink-field-names", "SitelinkFieldNames", "…")). Each renders a click.option between --fields and --dry-run (the position these commands use) and is parsed via the existing parse_nested_field_names, which rejects a provided-but-empty CSV with the same UsageError the inline code raised. Merged into the request params after the common params.
• ids_criteria_key (default "Ids") — SelectionCriteria key for the default --ids builder; clients maps --ids to ClientIds.
• fields_help (default "Comma-separated field names") — resource-specific --fields wording; sitelinks uses "Comma-separated SitelinksSet FieldNames".

The shared get_options decorator (utils.py) gains matching nested_options / fields_help hooks, both defaulting to current behavior so its ~15 other @get_options callers are byte-identical.

adimages / retargeting (no nested fields, extra criteria) migrate with a small per-module criteria_builder; sitelinks / feeds / clients use the default builder plus nested_field_options (+ ids_criteria_key / fields_help where they diverge).

Byte-identity (hard invariant of #587)

Verified each command against pre-migration baselines — all 21 snapshots identical: --help (option order, the nested-field option position, the custom --fields help), --dry-run payloads (incl. the ClientIds key and 1/2/5 nested-field params), the provided-but-empty *FieldNames UsageError, and the empty/limit cases.

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 / carve-outs (deferred follow-up)

• adextensions — its inline params always emit an empty SelectionCriteria and order *FieldNames before Page; reproducing those exact wire bytes needs a quirk flag, so it's intentionally left for later rather than bandaided.
• leads / agencyclients — no --ids (bespoke required --turbo-page-ids / --logins+--archived); they'd need a future include_ids=False factory option.

🤖 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 36s —— View job

---

Review — PR #598: nested-*FieldNames get migration

• Read the PR diff (git diff origin/main...HEAD)
• Review factory changes (_get.py, utils.py)
• Review migrated modules (adimages, retargeting, sitelinks, feeds, clients)
• Check byte-identity / behavior preservation claims
• Post review feedback

---

Verdict: No critical issues found. This is a faithful, behavior-preserving refactor. Net −175 lines with the CLI surface and wire payloads preserved.

What I verified against the pre-migration code

Nested *FieldNames parsing & empty-CSV guard. The factory routes nested options through parse_nested_field_names → parse_field_names_option, which raises UsageError("Provide a non-empty comma-separated {wsdl_key} list.") when a flag is provided-but-empty (raw_value is not None and not parsed). With wsdl_key = SitelinkFieldNames / FileFeedFieldNames / UrlFeedFieldNames etc., the interpolated message is byte-identical to the inline checks it replaces — so the i18n catalog key is unchanged. ✅

Param ordering on the wire. Old code did params.update(parsed_nested) after build_common_params; the factory does the same (params.update(parse_nested_field_names(...)) at _get.py:159-163), iterating the nested tuple in declaration order. Key insertion order is preserved, so dry-run JSON matches. ✅

--ids → SelectionCriteria key. ids_criteria_key="ClientIds" correctly reproduces the clients-specific ClientIds key; all others default to Ids. ✅

--help option order. Decorator application order (--ids outermost → resource extra_options → get_options block with nested options rendered between --fields and --dry-run) reproduces the original explicit stacks for all five modules. ✅

kwarg routing. nested_specs derives the Click kwarg via flag[2:].replace("-", "_"), which is correct for every flag here (e.g. --contragent-tin-info-field-names → contragent_tin_info_field_names). The default criteria_builder lambda swallows the nested/extra kwargs via **_, and the custom _adimages_criteria / _retargeting_criteria builders accept their extras explicitly. ✅

Minor observations (non-blocking, no action required)

1. Validation order shifted slightly for feeds/sitelinks — the empty-CSV UsageError now fires after build_common_params rather than before it. Since these commands have no criteria_limits/require_criteria_message and criteria construction can't fail, the observable result (the same UsageError) is unchanged. Worth keeping in mind only if a future migrated command pairs nested fields with a criteria guard, where the relative order of the two errors would matter.

2. Latent factory limitation (not exercised here): a command using the default criteria builder together with extra_options would silently drop those extra values from the criteria, since the default lambda discards **_. None of the five migrated commands hit this (the two with extra criteria supply a custom builder), but it's a sharp edge for the next migration — a builder is required whenever extra_options feed SelectionCriteria.

3. The deferred carve-outs in the description (adextensions, leads, agencyclients) are correctly scoped out; nothing in this diff regresses them.

Note on testing

I could not execute pytest/the CLI in this review environment (sandbox blocked command execution). My byte-identity conclusions are from static analysis of the diff; they rely on the PR's stated CI run (2552 passed, including test_dry_run/test_api_coverage/test_comprehensive). I'd suggest confirming the test_wsdl_parity_gate / dry-run snapshot tests are green in CI before merge, since those are the authoritative guard for the wire-payload claims.
· branch feat/587-group2-nested-fields

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: 00bd655454

ℹ️ 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".