Skip to content

Validate set_color_depth input in the Python tracker - #381

Closed
snowplow-claude-review[bot] wants to merge 1 commit into
masterfrom
loop/3a307af295a281a9ba70f6d2fa691c78-snowplow-python-tracker-29727716248
Closed

Validate set_color_depth input in the Python tracker#381
snowplow-claude-review[bot] wants to merge 1 commit into
masterfrom
loop/3a307af295a281a9ba70f6d2fa691c78-snowplow-python-tracker-29727716248

Conversation

@snowplow-claude-review

Copy link
Copy Markdown

🤖 Draft implementation — refinement loop (3a307af295a281a9ba70f6d2fa691c78)

Drafted automatically by the Snowplow refinement loop (refine → product review → implement → review) from a Notion refinement, and opened here for a human to finish.

Heads-up: this is a starting point. A human reviewer should verify, finish, and run CI before merging.

What the loop did

Validate set_color_depth input in the Python tracker

What changed

Added greater_than(depth, 0) validation to Subject.set_color_depth in snowplow_tracker/subject.py, placed immediately before the assignment to standard_nv_pairs["cd"]. The greater_than contract helper was already imported on line 19 — no new imports or error-handling patterns introduced.

Updated the depth docstring type annotation from int to int,>0 to match the style of the sibling setters (set_screen_resolution, set_viewport).

Added test_set_color_depth_validation to TestSubject in snowplow_tracker/test/unit/test_subject.py:

  • Asserts ValueError for 0 and -1
  • Confirms positive values (24) still work and are stored correctly

All 4 existing subject tests continue to pass (test_subject_0, test_combine_subject, etc.).

Why

set_screen_resolution and set_viewport already guard their numeric inputs with greater_than, but set_color_depth was never given the same treatment. Zero and negative colour-depth values are nonsensical and would silently produce invalid events. This is a consistency fix — no wire-format or schema change; the cd field itself is unchanged.

Decisions

  • Reused the existing greater_than contract helper (no new error handling invented).
  • No change to any other Subject setter — out of scope by design.
  • No schema or wire-format change — only invalid inputs now raise at the SDK boundary.

Reviewer checklist

  • greater_than(depth, 0) is placed before the assignment in set_color_depth, consistent with sibling setters
  • Test covers both invalid inputs (0, -1) and a valid one (24)
  • Existing tests still pass (test_subject_0, test_combine_subject)
  • No unintended changes to other setters or to the event payload format

…2fa691c78-snowplow-python-tracker-29727716248)
@snowplowcla

Copy link
Copy Markdown

Thanks for your pull request. Is this your first contribution to a Snowplow open source project? Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://docs.snowplowanalytics.com/docs/contributing/contributor-license-agreement/ to learn more and sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

@snowplowcla Snowplow CLA bot (snowplowcla) added the cla:no [Auto generated] Snowplow Contributor License Agreement has not been signed. label Jul 20, 2026
@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 96.63% (+0.01%) from 96.619% — loop/3a307af295a281a9ba70f6d2fa691c78-snowplow-python-tracker-29727716248 into master

@matus-tomlein
Matus Tomlein (matus-tomlein) deleted the loop/3a307af295a281a9ba70f6d2fa691c78-snowplow-python-tracker-29727716248 branch July 20, 2026 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla:no [Auto generated] Snowplow Contributor License Agreement has not been signed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants