Skip to content

Conversation

@octoaide
Copy link
Contributor

@octoaide octoaide bot commented Jan 14, 2026

Closes #275

Summary

This PR adds defensive validation to TimeSeries::try_new to ensure the provided period and interval values satisfy the required multiple relationship and to prevent invalid inputs (such as zero interval). The change documents the error conditions, adds unit tests, and updates the CHANGELOG.

What I changed and why

  • Validate inputs in TimeSeries::try_new:

    • Return an error if interval == 0 to avoid divide-by-zero.
    • Return an error if period is not a multiple of interval (enforces period % interval == 0 semantics using is_multiple_of()).
      These checks make the implicit assumption explicit and protect future changes from violating the constraint.
  • Add rustdoc to TimeSeries::try_new

    • Document the # Errors conditions (zero interval, non-multiple period, timestamp computation failure, usize overflow).
  • Add unit tests

    • try_new_valid_period_interval: valid (period is a multiple of interval) returns Ok.
    • try_new_invalid_period_not_multiple_of_interval: non-multiple returns an appropriate error.
    • try_new_interval_zero: zero interval returns an appropriate error.
  • Update CHANGELOG.md

    • Add an Unreleased entry describing the change.

Files changed

  • src/subscribe/time_series.rs (added validation, docs, tests)
  • CHANGELOG.md (documented change)

Quality assurance

  • All tests pass locally (including the 3 new unit tests).
  • Clippy linting applied; the multiple-check uses is_multiple_of() as suggested.

Notes

This is a defensive/validation change only — it does not alter existing happy-path behavior when callers already provide valid period/interval pairs. The goal is to make the constraint explicit and fail early with clear errors if it's violated.

 2 files changed, 89 insertions(+), 1 deletion(-)

Automated changes made by octoaide bot.
@octoaide octoaide bot requested a review from pott-cho January 14, 2026 04:34
@codecov
Copy link

codecov bot commented Jan 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 11.21%. Comparing base (4e1dd53) to head (7b7cee7).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #276      +/-   ##
==========================================
+ Coverage   10.19%   11.21%   +1.02%     
==========================================
  Files           8        7       -1     
  Lines         628      535      -93     
==========================================
- Hits           64       60       -4     
+ Misses        564      475      -89     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@octoaide
Copy link
Contributor Author

octoaide bot commented Jan 22, 2026

@kimhanbeom This PR (Add defensive validation to TimeSeries::try_new — closes #275) is approved and mergeable, and all CI checks have passed. It has been in this state for 7+ days since the approval by @pott-cho. Could you please merge it when convenient, or let us know if you're holding it for a release/timing reason? Summary: adds input validation (zero interval and non-multiple checks), rustdoc for errors, unit tests, and CHANGELOG entry. Thanks!

@kimhanbeom
Copy link
Contributor

@octoaide
This PR is intended to be merged after the additional reviews from @JonghoKim-jj, and @sophie-cluml are completed. Until then, no merge pings are necessary.

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.

Add defensive validation to TimeSeries::try_new

3 participants