Re-order removing and adding display layers#57
Re-order removing and adding display layers#57Tiomat85 wants to merge 2 commits intonion-software:masterfrom
Conversation
Tiomat85
requested review from
KRLango,
cmeyer,
jamesrussell216 and
lisham2000
There was a problem hiding this comment.
Can you provide an associated integration test that succeeds in cases where it used to succeed; fails in the case in question before the patch; and succeeds in all cases after the patch. Changed to draft until that is provided. The analogous commit 48b60bd includes a test.
Also, the commit message needs to follow commit message guidelines, similar to 48b60bd. "Fix issues with copying display layers when splitting SI sequences."
Also, it is helpful to put "Fixes #55" in the top comment for the PR. This will automatically add a link to the PR from #55. "Addresses #55" does not automatically add the link. GitHub docs for automatic linking.
Finally, I didn't test the code - I'm waiting for the integration test to do a full review; but the code so far looks ok at first glance.
Added Integration test checking sequences for sequence split. Test replicates the scenario as described in Issue 55, failing before the fix and working after.
|
Merged c4225f3 |
|
Issue verified as fixed. Display layers are now not copied, user can copy them afterwards. Thanks! |
5 participants
Fixes #55
Fix issues with copying display layers when splitting SI sequences. Moved across the fix from cmeyer@48b60bd to the sequence_split so the display_layers object is used rather than cloned and attempted to be stitched back.