Remove _fullInput and other dead transforms code#7834
Conversation
camdecoster
left a comment
There was a problem hiding this comment.
Looks good! Two things:
- Could you add a draftlog? Since this removes the deprecated code, it would be good to note that.
- Could you do a sweep for comments that mention transforms or
_fullData?
Sure, I can add a draftlog, it could help with debugging later, so not a bad idea. But the removed code was already inaccessible so I don't expect any user-facing changes.
Good idea! 0 matches for |
|
|
||
| // disable 1D transforms (for now) | ||
| // Ensure _length is defined | ||
| traceOut._length = null; |
There was a problem hiding this comment.
all these traceOut._length = null statements refer to disabling transforms... I'm pretty sure they can all be removed after #7240, specifically these lines.
There was a problem hiding this comment.
@alexcjohnson Thanks, that's helpful. I considered removing them but wasn't confident, since I do see trace._length referenced in some places that are unrelated to transforms. But I think _length being undefined vs. null would behave identically in all of those cases anyway. So I'll try just removing all of the traceOut._length = null statements and make sure everything passes.
There was a problem hiding this comment.
@alexcjohnson Whoops, looks like we do actually specifically test for trace._length being numeric or null 😅
Does anything in that test or comment change your opinion about removing? Or should we just delete that part of the test?
There was a problem hiding this comment.
Ah ok - the "simplifying calc/plot logic later on" part I can get behind, and there is one danger in leaving it undefined rather than setting it explicitly to null: the relinkPrivateKeys step can in principle bring back in an old _length that was valid for the data that used to be in a trace but has since changed. So yeah, let's leave these in place unless and until we figure out which trace types actually have such logic.
|
LGTM! Just a couple more items I bet we could remove, as noted above. |
7892b10 to
b1dd513
Compare
Addresses #7250
trace._fullInputfield existed solely for keeping track of the original input trace after applying defaults but before applying transformstransformsfrom the API #7240, Remove transforms from bundles #7254),trace._fullInputis the same astracetrace._fullInputto simplytrace, and adjusts logic accordinglytrace._indexToPointswhich was likewise only used for transformstrace._fullInputortransformsThis PR should have zero user-facing impact.