General code review

[eparejatobes]

Will continue later.

[eparejatobes]

@laughedelic I'm pointing to stuff that needs to be reviewed with TODO single-line comments.

[laughedelic]

Ok, I will review it later.

[eparejatobes]

Done for now.

[laughedelic]

I replied just to some comments, will continue tomorrow

[eparejatobes]

@laughedelic please re-assign me once you finish reading everything here

[laughedelic]

I fixed some comments and replied here to others. There is more to fix, I will continue tomorrow. To see my replies, please, unfold them, because they got hidden after my commit.

[laughedelic]

Here's the full list of all TODO comments introduced in this PR (+ some of my old comments). Most are addressed, some refactoring-related ones are still pending. I will continue with this a bit later. @eparejatobes please, read my replies that are linked in this list for your convenience. Some of them are questions.

---

• [x] csv.scala:21: TODO: rewrite this with product types: (fixed in 3b8832b00d8e2c0be6b9794ecc912b8522e226bc..c4cc4ef9e1a22a934668a47b5e123be3c1bac8ab)
• [x] csv.scala:26: TODO better Taxa/Taxon (done in 0eecd4693521504f4875116baa2caa93758c83c7)
• [x] package.scala:23: TODO why this? (replied in https://github.com/ohnosequences/mg7/pull/84#discussion_r72090408)
• [x] package.scala:26: TODO why not create an ordering from f and use std library? (replied in https://github.com/ohnosequences/mg7/pull/84#discussion_r72090731)
• [x] package.scala:34: TODO why not as ops? (replied in https://github.com/ohnosequences/mg7/pull/84#discussion_r72090593, also 9bb67c4a821fc799b73e9d64ba10325bc2bc8bf5)
• [x] parameters.scala:100: TODO why default values? not good (replied in https://github.com/ohnosequences/mg7/pull/84#discussion_r72091062, changed in 9bb67c4a821fc799b73e9d64ba10325bc2bc8bf5)
• [x] referenceDB.scala:11: TODO: the non-bundle part of the trait could be put in the blast-api lib (replied in https://github.com/ohnosequences/mg7/pull/84#discussion_r72473861)
• [x] taxonomyTree.scala:47: TODO why this?? +1?? (replied in https://github.com/ohnosequences/mg7/pull/84#discussion_r71976744, also 9bb67c4a821fc799b73e9d64ba10325bc2bc8bf5)
• [x] full.scala:18: TODO this description is outdated right? assignment is done per chunk IIRC (fixed in 9bb67c4a821fc799b73e9d64ba10325bc2bc8bf5)
• [x] noFlash.scala:18: TODO this description is outdated right? assignment is done per chunk IIRC (fixed in 9bb67c4a821fc799b73e9d64ba10325bc2bc8bf5)
• [x] noFlash.scala:45: TODO what's this supposed to be doing? (replied in https://github.com/ohnosequences/mg7/pull/84#discussion_r72075851)
• [x] noFlash.scala:76: TODO there are a lot of similar anonymous functions like this one which should use braces, so that the scope is clear (replied in https://github.com/ohnosequences/mg7/pull/84#discussion_r72076453)
• [ ] noFlash.scala:79: TODO factor this into a (private) method, same for listing objects etc
• [x] 1.flash.scala:2: TODO in general I prefer imports to occupy less space (fixed by @eparejatobes)
• [x] 1.flash.scala:11: TODO move all these "tool" bundles to a separate file (fixed in 9bb67c4a821fc799b73e9d64ba10325bc2bc8bf5)
• [ ] 1.flash.scala:25: TODO FLASh stuff change options, derived from reads type (¿is it my note?)
• [x] 4.assign.scala:23: TODO why override val here? why this is not a param? (fixed in 9bb67c4a821fc799b73e9d64ba10325bc2bc8bf5)
• [ ] 4.assign.scala:57: TODO this is too big. Factor BBH and LCA into methods
• [ ] 5.merge.scala:18: TODO no default arguments please
• [x] 5.merge.scala:20: TODO foreach on option :| (replied in https://github.com/ohnosequences/mg7/pull/84#discussion_r71976782)
• [x] 5.merge.scala:23: TODO as in other places, use {} here (replied in https://github.com/ohnosequences/mg7/pull/84#discussion_r71976795)
• [ ] 6.count.scala:79: TODO create a local case class or record for the return type
• [x] 6.count.scala:104: TODO why mutable Map?? (replied in https://github.com/ohnosequences/mg7/pull/84#discussion_r72159032)
• [x] 6.count.scala:121: TODO all this file output format-related code should be part of global configuration (replied in https://github.com/ohnosequences/mg7/pull/84#discussion_r72086940)
• [x] 6.count.scala:132: TODO all this file output format-related code should be part of global configuration (replied in https://github.com/ohnosequences/mg7/pull/84#discussion_r72086940)
• [ ] 6.count.scala:183: TODO use data records directly instead of this ugly tuple business
• [x] 7.stats.scala:23: TODO why io.Source here?? (fixed in 9bb67c4a821fc799b73e9d64ba10325bc2bc8bf5)
• [x] 7.stats.scala:44: TODO the whole method is a bit primitive (replied in https://github.com/ohnosequences/mg7/pull/84#discussion_r72089943)
• [x] 7.stats.scala:50: TODO: use csv.Row here (fixed in 3b8832b00d8e2c0be6b9794ecc912b8522e226bc)
• [x] 8.summary.scala:19: TODO why io.Source?? (fixed in 9bb67c4a821fc799b73e9d64ba10325bc2bc8bf5)

[eparejatobes]

OK @laughedelic thank you for https://github.com/ohnosequences/mg7/pull/84#issuecomment-235938466!

I replied to some. There's still some work to do there, so reassigning back to you.

[eparejatobes]

Also If you want, we could create issues for what's left and merge this.

[laughedelic]

Also If you want, we could create issues for what's left and merge this.

Sounds good. There are mostly refactoring issues left. I will open issues for them and merge this

[eparejatobes]

LGTM please @laughedelic add those issues we were talking about

[laughedelic]

Well, I wanted to fix here some of your newly commented minor things "/ It will go somewhere else then.

UPD: It's quite pointless having assignments and pullaprove integration when in the end you just come and merge it.