Add Bio::Seq::RichSeq to trunc() for not using clone() #106
Conversation
…marySeqI->trunc() not to do a clone of the original object first. The clone was cloning features of the rich sequence object outside the requested range in trunc()
|
Hello Matthew, before merging your pull request, could you add a simple example case to the other trunc() tests in t/Seq/PrimarySeq.t ?? The test case will help to avoid the reappearing of the issue in future commits. Also, could you change the Tab in the line for whitespaces? Tabs are incovenient because their lengths are displayed different in different programs. ;) |
…adn't changed my configs after reinstalling my laptop last month. - Changed line in trunc testing if an object is a Bio::Seq::RichSeq so it uses spaces for indentation rather than a tab character.
|
Sigh, didn't notice I hadn't updated my .emacs after reinstalling my laptop last month. Ok, the tab issue should be fixed. As for testing, I just emailed Chris with this, but I'm pondering what might be best. In t/Seq/PrimarySeq.t where you suggested, we'd have to create a RichSeq object with features, call trunc, then... check if the features in the returned object vanished? That seems shady and not very elegant, it'd also break the concept of PrimarySeq.t testing PrimarySeq objects by creating a RichSeq in there. Or make a RichSeq.t... for one test which can be easily documented in trunc why RichSeq is in if statement condition? I'll craft something if you really want it, but I'm not sure if it's needed. |
|
First, thanks for removing the tab. Ok, I get your reasoning on why PrimarySeq.t may not be the best place and I think you are right. But, there is an alternative place where I think it would fit in, "t/Seq/Seq.t" already have a section with specific RichSeq object tests (line 170, "# tests for RichSeq"), where you can adapt the object there and add the test (that would also avoid creating a 1-test file as you say). I think the test is still desireble because if the unintended behaviour was there, it was because there was no test keeping an eye on it. What do you think?? |
|
Yup, you've sold me. Hadn't had time to dig enough and notice RichSeq in Seq.t. I'll do that now. |
…Bio::Seq::RichSeq type object
|
Alright, tests created. Let me know if there's anything else I can do. |
|
Now it looks great and it passes all tests, so I will proceed with the merge. |
Add Bio::Seq::RichSeq to trunc() for not using clone()
Using clone() in Bio::PrimaryI->trunc() on a Bio::Seq::RichSeq type has an unintended consequence of cloning the entire sequence, including features outside the target range. See issue #83