refactor: migrate bids/bidmodifiers get to make_get_command (completes #587)

[axisrow]

Что

Завершает #587 — переводит последние две мигрируемые get-команды на общую фабрику make_get_command, переиспользуя её существующие хуки criteria_limits, require_criteria_message и nested_field_options. Новых параметров фабрики не добавлено.

• bids get — criteria-limit + require-at-least-one-filter, include_ids=False.
• bidmodifiers get — criteria-limit + 13 опциональных вложенных *FieldNames + дефолтный --levels в criteria.

Инварианты (байт-идентичность)

--help (порядок опций, 13 вложенных *FieldNames, choice/default у --levels) и --dry-run payload'ы (enforcement лимитов criteria, UsageError require-filter у bids get, всегда-присутствующий Levels и вложенные параметры) — байт-идентичны эталонам до миграции (проверено на 8 кейсах через CliRunner на свежем коде репозитория). Net −80 строк (по коду — сильнее в минус).

Carve-outs (осознанно оставлены hand-rolled)

Закрыта мигрируемая поверхность #587. Три оставшихся get задокументированы как carve-out'ы (в docstring _get.py + CHANGELOG), каждый — по структурной причине, которую фабрика намеренно не кодирует:

• leads get — --datetime-from/--datetime-to между --limit и --fetch-all (нестандартный порядок опций, общий get_options рендерит --limit/--fetch-all смежно).
• keywordbids get — вложенные SearchFieldNames/NetworkFieldNames несут собственные дефолты и эмитятся всегда (фабрика делает provided-only проекции).
• reports get — кастомный non-RPC TSV-стрим, не JSON-RPC get.

Проверки

• ruff чисто; полный офлайн-набор 2552 passed; гейты зелёные (dry_run, api_coverage, read_cassettes, wsdl_parity_gate, i18n, smoke_matrix).
• /simplify (4 агента: reuse / simplification / efficiency / altitude) — все чисто, правок не потребовалось.

Closes #587

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

---

Code Review — bids/bidmodifiers → make_get_command

• Read the diff (_get.py, bids.py, bidmodifiers.py)
• Verify byte-identity claims (option order, criteria/nested-param assembly)
• Check for bugs, logic errors, data-loss risks
• Post review

Verdict: no critical issues found. This is a behavior-preserving refactor. I focused only on functional correctness, not style.

What I verified

bids get (include_ids=False)

• Old order was build criteria → enforce limits → require-filter guard; the factory does the same (_get.py:186–192). svc = group.name → command_name="bids get" matches the old literal. ✅
• The require_criteria_message is passed as a plain string and the factory wraps it in t(...) (_get.py:192), matching the old t(...) call exactly. ✅
• _bids_get_criteria keeps a vestigial ids=None param, but with include_ids=False no --ids option is rendered, so ids is always None and never reaches the criteria. Harmless. ✅

bidmodifiers get

• Levels is always emitted (criteria = {"Levels": [...]}), so criteria is never empty — correctly no require_criteria_message. ✅
• Param assembly order preserved: build_common_params(...) then params.update(parse_nested_field_names(...)) (_get.py:199–206) mirrors the old params.update(parsed_nested). The 13 nested *FieldNames still land after the common params. ✅
• Nested kwarg mapping is correct: flag[2:].replace("-","_") → e.g. --ad-group-adjustment-field-names → ad_group_adjustment_field_names, pulled from **kwargs. ✅

Import hygiene (the main runtime risk in this kind of refactor)

• Confirmed no dangling references to the removed symbols (client_from_ctx, build_common_params, enforce_criteria_array_limits, get_default_fields, get_options, parse_csv_strings, format_output, parse_nested_field_names, Any) anywhere in either file.
• Remaining imports (t, handle_api_errors, create_client, parse_ids, parse_csv_upper) are still used by the set/lifecycle commands — no F401 unused-import that would trip the lint gate. ✅

One minor, non-blocking observation (error precedence)

In the old bidmodifiers get, parse_nested_field_names(raw_nested) ran before enforce_criteria_array_limits. In the factory the order is reversed — criteria-limit enforcement (_get.py:187) runs before nested-field parsing (_get.py:206).

Effect: if a user simultaneously supplies an over-limit criteria array and a provided-but-empty nested CSV (e.g. --video-adjustment-field-names ""), the surfaced UsageError is now the criteria-limit one instead of the empty-nested-field one. Both inputs are still rejected — no data loss, no silent acceptance — only which error message appears first changes, and only when both are invalid at once. Not worth changing unless a dry-run test pins that precedence.

Note on verification

I reviewed statically and via grep; I could not execute pytest/CLI dry-runs in this environment (sandbox blocked pip install/CLI invocation). The byte-identity claims rest on the author's 8-case CliRunner check plus the green dry-run/api-coverage/wsdl-parity gates — those are the right gates for this, and the code paths I traced are consistent with them.
· refactor/issue-587-bids-bidmodifiers-get

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: 5f84245d00

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