Fix WorkingMemoryDataModel and FlatWorkingMemoryDataModel to slurp from :src into vwmem
#7
Conversation
…rp` from `:src` into `vwmem`
| (sp/load-data DM (context :ROOT) slurp-able-data) | ||
| (sp/load-data DM (context :A/a) more-slurp-able-data) | ||
|
|
||
| (assertions "Data loaded in" |
There was a problem hiding this comment.
When writing tests, the general structure of english should compose into a sentence-like thing, when possible, for clarity about what you're testing.
" DOES WHAT?" In this case it ends up reading "Working memory model load data Data loaded in."
But that tells me, the person that has to debug things when the test breaks, nothing about what you expect. Does it merge it at the top level, or does it replace the entire data model?
It also has the effect of making you think about what it should do, and perhaps this leads you to look at the spec (the W3C one for scxml). Maybe there are requirements, perhaps it can simply be invented by the implementor....in either case saying what you expect it to do is pretty important, and often leads to additional assertions, each of which often needs its own textual clause to clarify which of the expected behaviors you are proving.
As an additional note it is important for this language to say what it does and not the reverse. Yeah, it doesn't paint my house red or launch a rocket...do I list all of the things it doesn't do? Proving negativity is very hard (you could screw up your measurement even, and see that "nothing" happened, but later find it was because you were looking for "nothing" in the wrong place).
Whereas, if you follow the rule of making assertions that actually expect a positive result (i.e. see a particular visible change) then you avoid that pitfall. So instead of "doesn't overwrite existing values" I would encourage "leaves existing, non-conflicting, values in place". In this case the assertion is the same, but the language reminds you of the pitfall. Or, in the case of our (potentially incorrect) implementation "overwrites the data that was in place"
There was a problem hiding this comment.
OK, so when I re-read https://www.w3.org/TR/scxml/#datamodel and consider the implications:
- The
datamodelnode can have multipledatachildren. - EACH child can have a
srcattribute
THEN it makes NO sense for loads to be a complete destruction of the data (in either of our data model implementations)
As an extension of the standard, we could allow an additional property on a data node to indicate a desired behavior (overwrite vs merge), but I think the default should be merge.
| (sp/get-at DM (context :ROOT) [:a :c]) => 100)))) | ||
| (sp/get-at DM (context :ROOT) [:a :c]) => 100)) | ||
|
|
||
| (component "load-data" |
| @@ -65,7 +65,7 @@ | |||
| (if (map? data) | |||
| (do | |||
| (log/trace "Loaded" data "into context" state-id) | |||
| (vswap! vwmem ::data-model assoc state-id data)) | |||
There was a problem hiding this comment.
Wow, how much sleep did I have before writing that???
| @@ -156,7 +156,7 @@ | |||
| (if (map? data) | |||
| (do | |||
| (log/trace "Loaded" data "into context" state-id) | |||
| (vswap! vwmem ::data-model assoc state-id data)) | |||
| (vswap! vwmem assoc ::data-model data)) | |||
There was a problem hiding this comment.
Um, is this right? Why not assoc-in like before? I think in both cases I was shooting for (vswap! vwmem update ::data-model assoc state-id data).
There was a problem hiding this comment.
Oh wait...right, for the flat model it isn't supposed to have anything to do with the state in question.
There was a problem hiding this comment.
But my testing comment does beg the question: Should this be a merge? Should the load operation have such an option, even?
No description provided.