Skip to content
This repository has been archived by the owner on Feb 14, 2023. It is now read-only.

calculated_item helper #131

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

calculated_item helper #131

wants to merge 1 commit into from

Conversation

ccutrer
Copy link
Owner

@ccutrer ccutrer commented Dec 13, 2022

No description provided.

@ccutrer ccutrer requested a review from jimtng December 13, 2022 22:37
# @return [Core::Types::State]
#
# If a Proc is given, {Rules::Terse.calculated_item calculated_item}
# will be used to keep the item's state up to date.
Copy link
Collaborator

@jimtng jimtng Dec 14, 2022

Choose a reason for hiding this comment

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

An example demonstrating how to use this would be great. It could be a part of the main Builder module's description above, I just can't add a comment there.

@ccutrer
Copy link
Owner Author

ccutrer commented Dec 14, 2022

refs boc-tothefuture#459

rule name, id: id, script: script, binding: block.binding do
changed(*items)
run do
item.ensure.update(yield)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add a state check here, to ensure that none of the captured items are undef?

Copy link
Owner Author

Choose a reason for hiding this comment

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

the problem is it’s not so easy. I have to run the block essentially as an on_load in order to identify the items. but I can’t check their state is not nil until I know what items I’m looking at

I can’t even catch NoMethodError, because that will interrupt execution of the block, so I won’t know what other items were accessed

I could, only within the context of a capture_items block, instead of returning nil return a special object that delegates to nil, but swallows any NoMethodErrors. but that sounds super error-prone

I think a better option would be to allow explicitly specifying the dependencies, and then if you do that, have an option for require_state: true

WDYT?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm going to noodle on this a bit. even if you make the user have their block be written to be safe in the face of UnDefType, it would likely return nil, and it might be the case that when that happens they want to not update the item, instead of updating it to NULL. but automatic item capture is like 90% of the appeal of this feature

Copy link
Collaborator

Choose a reason for hiding this comment

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

waiting for the result of your noodling

number_item Furnace_DeltaTemp
number_item FurnaceSupplyAir_Temp, state: 80
number_item FurnaceReturnAir_Temp, state: 70
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the initial state here, and test the rule when the items involved are UNDEF/NULL.

@jimtng
Copy link
Collaborator

jimtng commented Dec 27, 2022

Would it work with something like this? Note that not all items involved are looked up through one execution path.

calculated_item(ItemTotal) do
  if Item5.state == 5
    Item1.state + Item2.state
  else
    Item2.state + Item3.state
  end
end

@ccutrer
Copy link
Owner Author

ccutrer commented Dec 28, 2022

No, it wouldn't, at least not as currently implemented. I've been thinking of bringing in an actual Ruby parser, instead of running the block. But that likely wouldn't handle item references from nested method calls. So then I'm back to explicitly naming dependencies, and then deciding which (or multiple) auto-detection strategies to allow.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants