Skip to content

Add bidmodifier operating system type flag (#254) - #310

Merged
axisrow merged 1 commit into
mainfrom
codex/issue-254-bidmodifiers-os-type
May 24, 2026
Merged

Add bidmodifier operating system type flag (#254)#310
axisrow merged 1 commit into
mainfrom
codex/issue-254-bidmodifiers-os-type

Conversation

@axisrow

@axisrow axisrow commented May 24, 2026

Copy link
Copy Markdown
Owner

Summary

  • add typed --operating-system-type for bidmodifiers add MOBILE_ADJUSTMENT and TABLET_ADJUSTMENT
  • emit top-level nested WSDL fields MobileAdjustment.OperatingSystemType / TabletAdjustment.OperatingSystemType
  • move the two bidmodifiers add: complete optional adjustment WSDL parity #254 audit rows from missing_followup to supported and add single-line README examples

Official docs checked: https://yandex.ru/dev/direct/doc/en/bidmodifiers/add

Verification

  • python3 -m pytest tests/test_dry_run.py -k "bidmodifiers_add_mobile_operating_system_type or bidmodifiers_add_tablet_operating_system_type or bidmodifiers_add_rejects_incompatible_extra_flags"
  • python3 scripts/build_wsdl_optional_field_audit.py --check
  • python3 -m pytest tests/test_wsdl_parity_gate.py
  • python3 -m pytest tests/test_dry_run.py
  • python3 -m black --check direct_cli/commands/bidmodifiers.py tests/test_dry_run.py tests/test_wsdl_parity_gate.py
  • python3 -m direct_cli.cli bidmodifiers add --help

Closes #254

Copilot AI review requested due to automatic review settings May 24, 2026 07:36
@axisrow

axisrow commented May 24, 2026

Copy link
Copy Markdown
Owner Author

@codex @claude review

@claude

claude Bot commented May 24, 2026

Copy link
Copy Markdown

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


Review

  • Read changed files and PR diff
  • Cross-check WSDL parity (MOBILE_ADJUSTMENT / TABLET_ADJUSTMENT field shape)
  • Verify dry-run tests cover the new flag and rejection cases
  • Verify audit file matches the new supported state
  • Post review

Verdict: LGTM — small, surgical, WSDL-correct

Correctness

  • WSDL shape verified against tests/wsdl_cache/bidmodifiers.xml: MobileAdjustmentAdd and TabletAdjustmentAdd declare OperatingSystemType as a direct sibling of BidModifier (minOccurs=0, nillable=true). The implementation at direct_cli/commands/bidmodifiers.py:254-259 places it at exactly that nesting level. ✓
  • Enum values: OperatingSystemTypeEnum in the WSDL is {IOS, ANDROID}. The Click choice (line 197) matches exactly, and .upper() on the way out normalizes user input. ✓
  • Silent-data-loss invariant (parity-gate rule Add 1Password auth and simplify build config #2): --operating-system-type on DESKTOP_ADJUSTMENT etc. raises UsageError via _reject_incompatible_extra_flags (line 240) — covered by the new desktop_result test case. ✓
  • Audit accounting: 2817 → 2815 missing_followup, 424 → 426 supported — math is consistent with the two rows moved out. ✓
  • OPTIONAL_FIELD_CLI_OPTIONS is extended in tests/test_wsdl_parity_gate.py:949 so the optional-field audit will treat both paths as covered. ✓
  • Scope check: bidmodifiers.set does not need the same flag — its WSDL BidModifierSetItem only contains Id + BidModifier (audit confirms: both supported via the minOccurs>=1 gate). Add-only scope is correct.

Minor nits (non-blocking)

  • _OPERATING_SYSTEM_TYPE_MODIFIERS (line 128) is defense-in-depth — by the time line 255 runs, _reject_incompatible_extra_flags has already guaranteed the type is one of those two. Harmless but slightly redundant; keeping it is defensible.
  • The new rejection test only exercises DESKTOP_ADJUSTMENT. A nice-to-have would be one more case where the parent type does have other allowed extras (e.g. --operating-system-type on DEMOGRAPHICS_ADJUSTMENT) — but the rejection helper is generic and the existing demographics rejection test already covers the symmetric direction, so this is fine.
  • OperatingSystemType is nillable="true" in the WSDL; the CLI only exposes set-or-omit, which is the right default for add (no need to send null when creating). Not in scope.

Tests

  • Happy paths: mobile (iosIOS) and tablet (ANDROID) — both confirm normalization and the exact nested payload shape.
  • Rejection path: covered as noted.
  • README updated in both EN and RU sections with a parallel dry-run example.

Nothing blocking. Clean implementation, WSDL-aligned payload, parity gate and audit kept in sync.
· branch codex/issue-254-bidmodifiers-os-type

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the bidmodifiers add CLI command to support the optional WSDL fields MobileAdjustment.OperatingSystemType and TabletAdjustment.OperatingSystemType by introducing a typed --operating-system-type flag, and updates the WSDL parity/audit artifacts and documentation accordingly.

Changes:

  • Added --operating-system-type (IOS|ANDROID) support for bidmodifiers add when --type is MOBILE_ADJUSTMENT or TABLET_ADJUSTMENT, emitting the nested OperatingSystemType field.
  • Added dry-run tests and parity-gate mappings to prevent silent data loss for these optional WSDL fields.
  • Updated the optional-field audit classification and README usage examples.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
direct_cli/commands/bidmodifiers.py Adds the new --operating-system-type flag, validates compatibility by modifier type, and emits OperatingSystemType in the nested adjustment object.
tests/test_dry_run.py Adds dry-run coverage for mobile/tablet OS type emission and validates incompatible-flag rejection for non-mobile/tablet types.
tests/test_wsdl_parity_gate.py Registers the new flag as the CLI option covering the corresponding optional WSDL paths.
tests/WSDL_OPTIONAL_FIELD_AUDIT.md Moves the two audited OS type rows from missing_followup to supported and updates summary counts.
README.md Adds a one-line example using --operating-system-type for bidmodifiers add.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Can't wait for the next one!

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

@axisrow
axisrow merged commit 49c36bd into main May 24, 2026
10 checks passed
@axisrow
axisrow deleted the codex/issue-254-bidmodifiers-os-type branch May 24, 2026 07:42
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.

bidmodifiers add: complete optional adjustment WSDL parity

2 participants