convert vis-type to visType at parse time #393
Conversation
jest coverage report 🧪Total coverage
|
src/simularium/VisData.ts
Outdated
| @@ -63,7 +62,9 @@ class VisData { | |||
| }; | |||
|
|
|||
| for (let k = 0; k < AGENT_OBJECT_KEYS.length; ++k) { | |||
| agentData[AGENT_OBJECT_KEYS[k]] = floatView[j++]; | |||
| const key = AGENT_OBJECT_KEYS[k]; | |||
| const targetKey = key === "vis-type" ? "visType" : key; | |||
There was a problem hiding this comment.
Question: does simulariumio output vis-type anymore? Will we ever see data with this key name? I am sort of hoping we can remove it forever. Also I suggest putting the if key is vis-type then rename it to visType into a single function so that vis-type is not repeated in more than one place. The function can have a pointed name like fixLegacyBadAgentPropertyName or something...
There was a problem hiding this comment.
@ascibisz what do you think about this?
There was a problem hiding this comment.
Simulariumio doesn't output anything with vis-type or visType in it, as it doesn't set key names in that part of the data at all, the type of data each number represents is assumed based on its position. So as long as all references to vis-type are changed to visType within this repo, you should be fine. I don't see any references to vis-type anywhere else.
There was a problem hiding this comment.
I see, so even when we stream a json trajectory, the agent data is just in an ordered array of numbers without key names..? Which means we can totally replace "vis-type" throughout this code!
There was a problem hiding this comment.
this line should probably change: https://github.com/simularium/octopus/blob/e4ef3da8b2c1f0ed1d01125194fd778a352cfa62/octopus/agent.py#L23
There was a problem hiding this comment.
Correct! I will change that line of code, to be safe, although it's not currently getting called anywhere in Octopus
| @@ -154,7 +154,6 @@ export interface FileReturn { | |||
|
|
|||
| // IMPORTANT: Order of this array needs to perfectly match the incoming data. | |||
| export const AGENT_OBJECT_KEYS = [ | |||
| // TODO: convert "vis-type" to visType at parse time | |||
There was a problem hiding this comment.
I feel like this key still needs some sort of comment here about how this is for json data coming from the server? or maybe not. Does octopus send json data out?
Yes, done! |
src/simularium/types.ts
Outdated
| @@ -154,7 +154,6 @@ export interface FileReturn { | |||
|
|
|||
| // IMPORTANT: Order of this array needs to perfectly match the incoming data. | |||
| export const AGENT_OBJECT_KEYS = [ | |||
| // TODO: convert "vis-type" to visType at parse time | |||
| "vis-type", | |||
There was a problem hiding this comment.
can this line go away now too - or change to visType?
Time Estimate or Size
small
Problem
There were todo comments in the code about converting the
["vis-type]"key inAgentDatatovisTypeSolution
Added checks in parsing functions to output visType properties when incoming data has vis-type properties. Trajectories are running and tests are passing.