Skip to content

What changed - #383

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

What changed#383
snowplow-claude-review[bot] wants to merge 1 commit into
masterfrom
loop/3a307af295a281a9ba70f6d2fa691c78-snowplow-python-tracker-29729203883

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

What changed

Added input validation to Subject.set_color_depth to reject non-positive values, consistent with the existing guards on set_screen_resolution and set_viewport.

snowplow_tracker/subject.py — inserted greater_than(depth, 0) as the first line of set_color_depth (before the field assignment), exactly mirroring lines 63–64 (set_screen_resolution) and lines 77–78 (set_viewport). No import changes needed — greater_than was already imported. Docstring type annotation updated from int to int,>0 for consistency with the sibling setters.

snowplow_tracker/test/unit/test_subject.py — added test_set_color_depth_invalid to TestSubject. Asserts pytest.raises(ValueError) for inputs 0 and -1, and asserts no error for input 1. Follows the pytest.raises pattern already in the file (e.g. line 69).

Why

The set_screen_resolution and set_viewport setters already guard their numeric inputs with the repo's greater_than contract helper. set_color_depth was the only numeric setter missing this guard, allowing nonsensical values (zero, negatives) to be silently attached to events. This change closes that gap with no new error-handling machinery — just the same one-liner already used elsewhere.

What a reviewer should verify

  • greater_than(depth, 0) is in the correct position (first line of set_color_depth, before the self.standard_nv_pairs['cd'] = depth assignment).
  • The docstring annotation reads int,>0, matching set_screen_resolution and set_viewport.
  • test_set_color_depth_invalid covers 0, -1, and a positive value (1), and uses pytest.raises(ValueError).
  • Existing tests test_subject_0 (depth=1080) and test_combine_subject (depth=10) still pass — no regression.

…2fa691c78-snowplow-python-tracker-29729203883)
@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-29729203883 into master

@matus-tomlein
Matus Tomlein (matus-tomlein) deleted the loop/3a307af295a281a9ba70f6d2fa691c78-snowplow-python-tracker-29729203883 branch July 20, 2026 11:48
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