search

kbaseapps / GenomeFileUtil

MIT License
0 stars 16 forks source link

Changing duplicate id behavior for Genbank files. #183

Closed slebras closed 3 years ago

slebras commented 4 years ago

Dupliate ids are now accepted. the string '.n' is appended where n = "the number of times the id has been previously seen."

jayrbolton commented 4 years ago

Need to update changelog and version

slebras commented 4 years ago

I set up the new test mainly to have a way to test the current file quickly. Another test module is probably not necessary for this update

I'm not sure how to get around testing this new behavior, other than by running the whole upload. We can definitely tone down the size of the input genbank file for speed

jayrbolton commented 4 years ago

I give my thumbs if you can reduce the size of both the test module and test data. I would also reduce all the test assertions to just what you need to prove for this case so it's not redundant. But if those reductions aren't possible then I give thumbs up as-is.

jayrbolton commented 4 years ago

What is the plan for this PR? I'm afraid this is going to idle and die, but seems like an important fix.

slebras commented 4 years ago

Revisiting this PR. Spent some time thinking about and trying a few things to reduce the size of the test, but ran up against my knowledge of Genbank files

jayrbolton commented 3 years ago

LGTM

jayrbolton commented 3 years ago

@jsfillman @jkbaumohl You guys have to approve this PR to unblock the merge button because at some point you were set as code owners.