search

simularium / simularium-viewer

NPM package to view Simularium trajectories in 3D
Apache License 2.0
2 stars 0 forks source link

Fix/plotdata #386

Closed meganrm closed 3 months ago

meganrm commented 3 months ago

Time Estimate or Size

xsmall

Problem

getPlotData is typed to return Plot[] but when I logged what I got, it returns an object with {version: 1, data: Plot[]}, so I changed the functions to actually return Plot[]

Solution

Adjust the return of getPlotData to unpack the data

Type of change

Please delete options that are not relevant.

github-actions[bot] commented 3 months ago

jest coverage report 🧪

Total coverage

Status Category Percentage Covered / Total
🔴 Statements 40.31% 2049/5083
🔴 Branches 42.89% 845/1970
🔴 Functions 36.69% 418/1139
🔴 Lines 40.52% 1962/4841

Status of coverage: 🟢 - ok, 🟡 - slightly more than threshold, 🔴 - under the threshold

meganrm commented 3 months ago

What about all the plots in our example trajectories that are displayed in the simularium-website? Do they go through a different code path than this? (would this change break them or are they already broken and this is fixing it?)

this is a breaking change, but the fix is really small: remove a .data when unpacking the data. I could have changed the typing but my thinking here is that the version is really for the internals of the viewer, and anyone consuming the plot data shouldn't be managing/caring about the version number

toloudis commented 3 months ago

this is a breaking change, but the fix is really small: remove a .data when unpacking the data. I could have changed the typing but my thinking here is that the version is really for the internals of the viewer, and anyone consuming the plot data shouldn't be managing/caring about the version number

So the fix goes in simularium-website, or where? Is there already another PR that fixes the breakage?

meganrm commented 3 months ago

this is a breaking change, but the fix is really small: remove a .data when unpacking the data. I could have changed the typing but my thinking here is that the version is really for the internals of the viewer, and anyone consuming the plot data shouldn't be managing/caring about the version number

So the fix goes in simularium-website, or where? Is there already another PR that fixes the breakage?

it's here https://github.com/simularium/simularium-website/pull/522

ShrimpCryptid commented 3 months ago

(Left an approval since I saw that this was urgent, but will defer to Dan since there was an existing discussion)