Skip to content

Normalize tag filters across stores#19

Closed
jolovicdev wants to merge 2 commits into
masterfrom
test/normalize-tag-filters
Closed

Normalize tag filters across stores#19
jolovicdev wants to merge 2 commits into
masterfrom
test/normalize-tag-filters

Conversation

@jolovicdev
Copy link
Copy Markdown
Owner

Summary

  • add a shared helper for normalizing tag filter input
  • use the same normalized filters in SQLite and Redis log/invalidate paths
  • skip backend delete work when a tag filter set is empty

Testing

  • uv run ruff check src/cashet/_client_base.py src/cashet/store.py src/cashet/redis_store.py

Copy link
Copy Markdown

@ds-review ds-review Bot left a comment

Choose a reason for hiding this comment

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

PR Review

PR: Normalize tag filters across stores

Important

Verdict: Request changes - 1 actionable finding, highest severity P0.

Findings (1)

P0 Critical Broken indentation in normalize_tag_filters

src/cashet/_client_base.py:93-95

The function body ends after the first if block because lines 93-95 are outdented to module level. This causes a NameError at import time for the bare if not tags: at module scope, or if that is bypassed the function returns None for truthy input, leading to AttributeError in callers.

How To Recheck

Reply @ds-review recheck under the relevant inline finding after pushing a fix.

Comment thread src/cashet/_client_base.py Outdated
) -> dict[str, str | None]:
if not tags:
return {}
return {str(key): str(value) for key, value in tags.items() if value is not None}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 High normalize_tag_filters drops None-valued tag existence filters

The dict comprehension filters out entries where value is None, but None indicates existence-only checks (key present, any value). This breaks tag filtering in list_commits and delete_by_tags because such filters become empty and are ignored.

Suggested change
return {str(key): str(value) for key, value in tags.items() if value is not None}
if not tags:
return {}
return {str(k): str(v) if v is not None else None for k, v in tags.items()}

Thanks

Co-authored-by: ds-review[bot] <279337960+ds-review[bot]@users.noreply.github.com>
@jolovicdev jolovicdev closed this May 8, 2026
@jolovicdev jolovicdev deleted the test/normalize-tag-filters branch May 10, 2026 23:26
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.

1 participant