New schema, variant context converter changes, and removal of old genoty... #97
Conversation
…otype converter code
|
One or more automated tests failed |
|
Do you want this merged into the topic branch, even with the failing tests? |
|
Yes, I believe that was the direction we decided is best (see PR #87) given the total size of all the variant changes. Doing both tasks of splitting up the changes into manageable chunks and keeping the build green will be near impossible, I think. |
|
Ah! OK, got it now. To confirm my understanding then, you are splitting the original PR into several smaller chunks, all of which will roll in to the topic branch? |
|
That's correct. Before the topic branch gets merged back into master, the build will be green and tests will all pass, of course. |
|
Sure, makes perfect sense now. However, from an organizational perspective, I think it should be possible to keep the tests green—I think this should be possible if all of the code you are replacing gets removed first, along with the tests that attach to this code. Throughout the set of PRs, you'll then get all the functionality replaced. Thoughts? |
|
That should also make the diffs a bit more straightforward. |
|
I would like to do this....BUT, I think this is why we switched to a topic branch to begin with. I'm actually not pulling code from all commits, or 1 squashed commit, but just the first of many. Keeping green would require digging through several commits, that sometimes spanned commits merged in from the Bigdatagenomics master. |
|
Sure, let's not worry about that for now. For future big changes though, that might be a good process to follow. |
| @@ -30,521 +31,61 @@ import fi.tkk.ics.hadoop.bam.VariantContextWritable | |||
| * If an annotation has a corresponding set of fields in the VCF standard, a conversion to/from the | |||
| * GATK VariantContext should be implemented in this class. | |||
| */ | |||
| class VariantContextConverter extends Serializable { | |||
| private[adam] class VariantContextConverter extends Serializable with Logging { | |||
| initLogging() | |||
There was a problem hiding this comment.
I think @jey did some cleaning a while back and identified that initLogging is superfluous.
|
The goal of this approach was to make it easier for @nealsid to share all his work in pieces. Let's not worry about green tests on a per-commit basis in this topic branch. We'll make sure that all tests pass (of course) before we merge the "vcf-work" branch to "master". |
|
@massie Sure, I understand that. My suggestion is for the next time we do a large feature on a new topic branch: we should start by deleting the code that will be replaced along with it's unit tests. Then, we won't struggle with diffs that have such massive amounts of deletions, and we'll be able to keep the unit tests green the whole route. |
|
One or more automated tests failed |
|
One or more automated tests failed |
New schema, variant context converter changes, and removal of old genoty...
|
Merging to allow better review of #98 |
...pe converter code