Add fluent time predicates: state_changed_within?, state_updated_within?, state_changed_since?, and state_updated_since?#536
Conversation
5773b11 to
f104d7f
Compare
changed_within?, updated_within?, changed_since?, and updated_since?last_updated_within?, last_changed_since?, and last_updated_since?
last_updated_within?, last_changed_since?, and last_updated_since?last_changed_within?, last_updated_within?, last_changed_since?, and last_updated_since?
last_changed_within?, last_updated_within?, last_changed_since?, and last_updated_since?last_changed_within?, last_updated_within?, last_changed_since?, and last_updated_since?
f104d7f to
2bc9fb4
Compare
There was a problem hiding this comment.
Pull request overview
Adds fluent, intention-revealing time predicates on Items to simplify checking whether an item’s state was changed/updated “within” a duration or “since” a timestamp, reducing ambiguity compared to manual timestamp comparisons.
Changes:
- Added
last_changed_within?,last_updated_within?,last_changed_since?, andlast_updated_since?(plusstate_*aliases) onOpenHAB::Core::Items::Item. - Added RSpec coverage for the new predicates, including argument validation and
#to_zoned_date_timeinteroperability. - Updated DSL persistence documentation examples to use timestamp-based calls (
*.ago) consistent with the persistence API.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lib/openhab/core/items/item.rb |
Introduces the new fluent time predicate APIs and aliases on Items. |
spec/openhab/core/items/item_spec.rb |
Adds specs validating predicate behavior and accepted argument shapes. |
lib/openhab/dsl.rb |
Adjusts persistence doc examples to pass timestamps (e.g., 1.hour.ago) and correct predicate naming. |
Comments suppressed due to low confidence (2)
lib/openhab/core/items/item.rb:165
- The documentation note for
last_changed_since?says it uses#last_state_update, but the implementation compares againstlast_state_change. Please update the note to reference#last_state_change(and keep the persistence distinction).
# Check if the item's state has changed since a certain time (inclusive).
#
# Note: This method uses the Item's {#last_state_update} timestamp, which is an internal
# timestamp maintained within the Item object, independent of any persistence data.
# This is different from {Persistence#changed_since?}, which is based on Persistence data.
lib/openhab/core/items/item.rb:155
- The ArgumentError message says the argument must be a "Duration", but this method accepts any TemporalAmount that responds to
#ago(includingjava.time.Periodfrom1.day). Consider updating the message (and corresponding spec) to match the actual accepted type.
def last_updated_within?(duration)
raise ArgumentError, "duration must be a Duration" unless duration.respond_to?(:ago)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2bc9fb4 to
8fd8c5b
Compare
last_changed_within?, last_updated_within?, last_changed_since?, and last_updated_since?state_changed_within?, state_updated_within?, state_changed_since?, and state_updated_since?
7cc90f6 to
c983761
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
lib/openhab/core/items/item.rb:178
state_changed_since?will raise iflast_state_changeisnil(no prior change). Predicate methods should generally be safe to call; consider treating a nil timestamp as "has not changed since" and returningfalse.
def state_changed_since?(time)
last_state_change >= time
end
lib/openhab/core/items/item.rb:192
state_updated_since?will raise whenlast_state_updateisnil(no prior update). Consider returningfalsein that case so callers don't need to special-case initialization.
def state_updated_since?(time)
last_state_update >= time
end
c983761 to
3de8468
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
lib/openhab/core/items/item.rb:167
- This method unconditionally calls
last_state_change, which is only present in openHAB 5.0+. Since the library still supports older versions (e.g., therespond_to?(:last_state)guard in#inspect), consider checkingrespond_to?(:last_state_change)first to avoidNoMethodErroron openHAB 4.3.
def state_changed_since?(time)
changed_at = last_state_change
return false if changed_at.nil?
changed_at >= time
lib/openhab/core/items/item.rb:203
- This predicate calls
last_state_updatewithout checking availability. If run against openHAB versions prior to 5.0, this will raiseNoMethodError. Consider arespond_to?(:last_state_update)check (consistent with the existing OH 4.3 compatibility guard in#inspect).
def state_updated_since?(time)
updated_at = last_state_update
return false if updated_at.nil?
updated_at >= time
…hin?`, `last_changed_since?`, and `last_updated_since?` Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
3de8468 to
d300161
Compare
|
I'm having second thoughts on this. Perhaps we should add something more generic against all date/time stuff instead. e.g. MyItem.last_state_change.within?(5.minutes)
MyItem.last_state_change.before?(SunsetTime.state)Essentially date_time1.within?(duration)
date_time1.before?(date_time2)
date_time1.after?(date_time2) |
|
superseded by #538 |
I must admit, every time I wrote
Item.last_state_change > 1.hour.agoI had to slow down and think carefully whether it's what I actually wanted.Hopefully these fluent predicates make it easier to write / reason about.