Skip to content

Share task timing option parsing#18

Closed
jolovicdev wants to merge 2 commits into
masterfrom
test/cache-ttl-cleanup
Closed

Share task timing option parsing#18
jolovicdev wants to merge 2 commits into
masterfrom
test/cache-ttl-cleanup

Conversation

@jolovicdev
Copy link
Copy Markdown
Owner

Summary

  • centralize conversion of numeric task timing options into timedelta values
  • reuse the same parsing path when importing exported task metadata
  • cover export/import preservation for timeout and ttl settings

Validation

  • uv run ruff check src/ tests/
  • uv run pyright src/
  • uv run pytest tests/ -v

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: Share task timing option parsing

Important

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

Findings (1)

P1 High duration_from_seconds converts 0 to None

src/cashet/_client_base.py:59

The function uses truthiness (if seconds) to decide whether to return a timedelta. Passing 0 or 0.0 evaluates to False, so the function returns None instead of timedelta(0). This silently changes the behavior from the previous explicit None check, where a zero timeout or TTL was preserved. A zero timeout can be meaningful (e.g., no delay or immediate expiration), so collapsing it to None is incorrect.

How To Recheck

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

Comment thread src/cashet/_client_base.py Outdated


def duration_from_seconds(seconds: int | float | None) -> timedelta | None:
return timedelta(seconds=seconds) if seconds else 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 duration_from_seconds converts 0 to None

The function uses truthiness (if seconds) to decide whether to return a timedelta. Passing 0 or 0.0 evaluates to False, so the function returns None instead of timedelta(0). This silently changes the behavior from the previous explicit None check, where a zero timeout or TTL was preserved. A zero timeout can be meaningful (e.g., no delay or immediate expiration), so collapsing it to None is incorrect.

Suggested change
return timedelta(seconds=seconds) if seconds else None
return timedelta(seconds=seconds) if seconds is not None else None

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@ds-review recheck

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Recheck: resolved. The duration_from_seconds function now uses if seconds is not None instead of truthiness, so passing 0 returns timedelta(0) instead of None.

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.

Automated PR review encountered an error and could not complete.

@jolovicdev jolovicdev closed this May 8, 2026
@jolovicdev jolovicdev deleted the test/cache-ttl-cleanup 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