Closed cannawen closed 1 year ago
Wait, let's bring this back. I don't like storing default values on the Rule object because it's super mysterious and non-obvious who is setting the default values
The test problem has been fixed by using a real Engine in our tests I don't think saving stuff to the disk was ever a problem.
OK, saving to disk was a problem. Fixable though.
This may lead to weird things down the line where undefined is conflated with the default value. We will never be able to write the default value to disk, unless when we get a new fact, we EXPLICITLY CHECK IF IT IS THE DEFAULT VALUE. IF THE DB FACT IS UNDEFINED AND WE ARE PASSING THE DEFAULT VALUE, WRITE IT TO DB!!! <---todo; also create a test for this
A bit of strange stuff happening in Engine now. If fact has not changed, we are still saving the fact because JUST MAYBE the underlying db has a fact of undefined and we are trying to set it to the default value. The db will never return undefined if the fact has a default value.
It really shouldn't be the db's job to give back the default value, but I don't want to check for defaults everywhere... Perhaps engine.get can check for defaults and have nobody use the db.get ?
Moving default value logic checking onto Engine instead of FactStore.
Not 100% sure it belongs here (it's a weird static function because it doesn't depend on any of the Engine's state) but we'll roll with it for now.
Commits of interest if we want to revert this change in the future - 1df1594c3bb2feb047a358ba0aa66ab6d3442197 (have Engine handle defaults) ec89c7c1db91d4276fda69a6f6e651279282fc8c (have FactStore handle defaults)
Probably not worth it, makes testing harder and re-constituting facts from disk
patch.txt