Skip to content

Add input validation to Subject.set_color_depth - #384

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

Add input validation to Subject.set_color_depth#384
snowplow-claude-review[bot] wants to merge 1 commit into
masterfrom
loop/3a307af295a281a9ba70f6d2fa691c78-snowplow-python-tracker-29740085620

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

Add input validation to Subject.set_color_depth

What changed

Two files modified:

snowplow_tracker/subject.py

  • Added greater_than(depth, 0) as the first line of set_color_depth, before the assignment to self.standard_nv_pairs["cd"].
  • Updated the docstring :type depth: annotation from int to int,>0 to match the sibling setters.
  • The greater_than helper was already imported; no new imports needed.

snowplow_tracker/test/unit/test_subject.py

  • Added test_set_color_depth_rejects_non_positive which asserts that set_color_depth(0) and set_color_depth(-1) each raise ValueError (via pytest.raises), and that set_color_depth(1) succeeds.

Why

set_screen_resolution and set_viewport both guard their numeric inputs with greater_than; set_color_depth was the only sibling setter without this guard. Zero and negative colour-depth values are nonsensical and should never reach the wire. This is a consistency fix with no wire-format or schema change — the cd field itself is unchanged.

Key decisions

  • Reused greater_than exactly — no bespoke validation logic. The helper already raises ValueError on non-positive inputs, which is the repo's established contract.
  • Placement: guard fires before the assignment, mirroring the identical pattern in set_screen_resolution and set_viewport.
  • Scope kept minimal: docstring updated for accuracy; nothing else touched.

What to verify before merging

  • greater_than(depth, 0) is the correct call signature (check contracts.py — should match usage in sibling setters).
  • The new test runs cleanly alongside the existing test_subject.py suite (pytest snowplow_tracker/test/unit/test_subject.py).
  • No downstream code passes 0 or a negative to set_color_depth intentionally (a quick repo-wide grep for set_color_depth is sufficient).
  • Existing tests still pass (./run-tests.sh).

…2fa691c78-snowplow-python-tracker-29740085620)
@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.629% (+0.01%) from 96.619% — loop/3a307af295a281a9ba70f6d2fa691c78-snowplow-python-tracker-29740085620 into master

@matus-tomlein
Matus Tomlein (matus-tomlein) deleted the loop/3a307af295a281a9ba70f6d2fa691c78-snowplow-python-tracker-29740085620 branch July 27, 2026 07:51
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